Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use tags in agent metrics #950

Merged
merged 2 commits into from
Jul 24, 2018
Merged

Use tags in agent metrics #950

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b05f189
Change to tagged metrics in agent
eundoosong Jul 23, 2018
261f6f3
Move tc-reporter into Namesapce and rename
Jul 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions cmd/agent/app/processors/thrift_processor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package processors

import (
"errors"
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -196,7 +195,7 @@ func assertProcessorCorrectness(
metricsFactory *metrics.LocalFactory,
sizeF func() int,
nameF func() string,
counterType string,
format string,
) {
// wait for server to receive
for i := 0; i < 1000; i++ {
Expand All @@ -219,11 +218,9 @@ func assertProcessorCorrectness(
}

// agentReporter must emit metrics
batchesSubmittedCounter := fmt.Sprintf("tc-reporter.%s.batches.submitted", counterType)
spansSubmittedCounter := fmt.Sprintf("tc-reporter.%s.spans.submitted", counterType)
mTestutils.AssertCounterMetrics(t, metricsFactory, []mTestutils.ExpectedMetric{
{Name: batchesSubmittedCounter, Value: 1},
{Name: spansSubmittedCounter, Value: 1},
{Name: "tc-reporter.batches.submitted", Tags: map[string]string{"format": format}, Value: 1},
{Name: "tc-reporter.spans.submitted", Tags: map[string]string{"format": format}, Value: 1},
{Name: "thrift.udp.server.packets.processed", Value: 1},
}...)
}
14 changes: 6 additions & 8 deletions cmd/agent/app/reporter/tchannel/reporter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,19 @@ const (

type batchMetrics struct {
// Number of successful batch submissions to collector
BatchesSubmitted metrics.Counter `metric:"batches.submitted"`
BatchesSubmitted metrics.Counter `metric:"tc-reporter.batches.submitted"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. there is no need to add tc-reporter explicitly like that, since you can pass that string as the first argument to Namespace in L76 (see point 3 below).
  2. since the metrics are changing anyway, maybe we should rename them to use underscores, as we did with other backend metrics?
  3. I think historically tc was used to refer to tcollector, which was the TChannel name of the collector service (now it's jaeger-collector). When we implement Refactor Agent configuration #927, there will be a different type of reporter, e.g. grpc-reporter. Also, in some cases we internally run the agent with two reporters (using OtherReporters configuration of the builder), I don't even know if they distinguish their metrics. So my suggestion, at minimum, to rename this to tchannel-reporter, to make it more obvious what we're talking about, but we also need to figure out how metrics will work after Refactor Agent configuration #927.

cc @black-adder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. since the metrics are changing anyway, maybe we should rename them to use underscores, as we did with other backend metrics?

I can change it using underscore like tchannel_reporter. Just curious, the reason I kept using - is normalizer in jaeger-lib already replaces them. Don't we need it?
https://github.com/jaegertracing/jaeger-lib/blob/master/metrics/prometheus/factory.go#L109

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I see that collector metrics are still using dots as separators, so never mind about understores

Copy link
Contributor Author

@eundoosong eundoosong Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks!

we also need to figure out how metrics will work after #927.

I'll keep eyes on this.


// Number of failed batch submissions to collector
BatchesFailures metrics.Counter `metric:"batches.failures"`
BatchesFailures metrics.Counter `metric:"tc-reporter.batches.failures"`

// Number of spans in a batch submitted to collector
BatchSize metrics.Gauge `metric:"batch_size"`
BatchSize metrics.Gauge `metric:"tc-reporter.batch_size"`

// Number of successful span submissions to collector
SpansSubmitted metrics.Counter `metric:"spans.submitted"`
SpansSubmitted metrics.Counter `metric:"tc-reporter.spans.submitted"`

// Number of failed span submissions to collector
SpansFailures metrics.Counter `metric:"spans.failures"`
SpansFailures metrics.Counter `metric:"tc-reporter.spans.failures"`
}

// Reporter forwards received spans to central collector tier over TChannel.
Expand All @@ -71,11 +71,9 @@ func New(
zClient := zipkincore.NewTChanZipkinCollectorClient(thriftClient)
jClient := jaeger.NewTChanCollectorClient(thriftClient)
batchesMetrics := map[string]batchMetrics{}
tcReporterNS := mFactory.Namespace("tc-reporter", nil)
for _, s := range []string{zipkinBatches, jaegerBatches} {
nsByType := tcReporterNS.Namespace(s, nil)
bm := batchMetrics{}
metrics.Init(&bm, nsByType, nil)
metrics.Init(&bm, mFactory.Namespace("", map[string]string{"format": s}), nil)
batchesMetrics[s] = bm
}
return &Reporter{
Expand Down
16 changes: 5 additions & 11 deletions cmd/agent/app/reporter/tchannel/reporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package tchannel

import (
"fmt"
"testing"
"time"

Expand Down Expand Up @@ -105,16 +104,11 @@ func submitTestJaegerBatch(reporter *Reporter) error {
return reporter.EmitBatch(batch)
}

func checkCounters(t *testing.T, mf *metrics.LocalFactory, batchesSubmitted, spansSubmitted, batchesFailures, spansFailures int, prefix string) {
batchesCounter := fmt.Sprintf("tc-reporter.%s.batches.submitted", prefix)
batchesFailureCounter := fmt.Sprintf("tc-reporter.%s.batches.failures", prefix)
spansCounter := fmt.Sprintf("tc-reporter.%s.spans.submitted", prefix)
spansFailureCounter := fmt.Sprintf("tc-reporter.%s.spans.failures", prefix)

func checkCounters(t *testing.T, mf *metrics.LocalFactory, batchesSubmitted, spansSubmitted, batchesFailures, spansFailures int, format string) {
mTestutils.AssertCounterMetrics(t, mf, []mTestutils.ExpectedMetric{
{Name: batchesCounter, Value: batchesSubmitted},
{Name: spansCounter, Value: spansSubmitted},
{Name: batchesFailureCounter, Value: batchesFailures},
{Name: spansFailureCounter, Value: spansFailures},
{Name: "tc-reporter.batches.submitted", Tags: map[string]string{"format": format}, Value: batchesSubmitted},
{Name: "tc-reporter.spans.submitted", Tags: map[string]string{"format": format}, Value: spansSubmitted},
{Name: "tc-reporter.batches.failures", Tags: map[string]string{"format": format}, Value: batchesFailures},
{Name: "tc-reporter.spans.failures", Tags: map[string]string{"format": format}, Value: spansFailures},
}...)
}