Add misc changes for metrics correctness #1453
Conversation
pmcollins
requested review from
bogdandrutu,
dmitryax,
james-bebbington,
nilebox,
owais,
pjanotti and
tigrannajaryan
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1453 +/- ##
==========================================
+ Coverage 91.42% 91.44% +0.01%
==========================================
Files 248 248
Lines 17219 17226 +7
==========================================
+ Hits 15743 15752 +9
+ Misses 1064 1063 -1
+ Partials 412 411 -1
Continue to review full report at Codecov.
|
internal/goldendataset/metric_gen.go
Outdated
| return NewMetricGenerator().GenMetricDataFromCfg(cfg) | ||
| } | ||
|
|
||
| type MetricGenerator struct { |
There was a problem hiding this comment.
Does this need to be exported?
internal/goldendataset/metric_gen.go
Outdated
| metricID int | ||
| } | ||
|
|
||
| func NewMetricGenerator() *MetricGenerator { |
There was a problem hiding this comment.
same, can this be package private?
|
@bogdandrutu Thanks for taking a look. I have added a more detailed explanation of the contents of this PR. |
| type metricGenerator struct { | ||
| metricID int | ||
| } | ||
|
|
||
| func newMetricGenerator() *metricGenerator { | ||
| return &metricGenerator{} | ||
| } |
There was a problem hiding this comment.
Is this whole type needed? You can just pass around metricID.
There was a problem hiding this comment.
metricID is a counter that gets incremented every time a new metric is generated. It's used to create unique metric names. I guess we could hold and increment the current count on the stack, but it would be a more complicated solution than this.
|
Rebase? |
Done. |
| @@ -172,13 +173,20 @@ func (mb *MockBackend) ConsumeMetricOld(md consumerdata.MetricsData) { | |||
| } | |||
| } | |||
|
|
|||
| type TraceDualConsumer interface { | |||
| consumer.TraceConsumer | |||
| consumer.TraceConsumerOld | |||
There was a problem hiding this comment.
Why do you need the Old interface?
There was a problem hiding this comment.
To answer this and the next question, the old interface is used by Prometheus and OpenCensus on the metrics side, and Zipkin on the tracing side.
There was a problem hiding this comment.
That is no longer true :) for 2 out of 3 :)
| @@ -243,29 +251,28 @@ func (tc *MockTraceConsumer) ConsumeTraceData(ctx context.Context, td consumerda | |||
| return nil | |||
| } | |||
|
|
|||
| type MetricsDualConsumer interface { | |||
| consumer.MetricsConsumer | |||
| consumer.MetricsConsumerOld | |||
| } | ||
|
|
||
| var _ TraceDualConsumer = (*MockTraceConsumer)(nil) | ||
|
|
||
| type MockTraceConsumer struct { |
There was a problem hiding this comment.
Does this need to be public?
There was a problem hiding this comment.
Yes, it will be used by the test harness in the metrics package as a placeholder trace consumer.
|
Please rebase |
Done. |
…1453) * Bump github.com/stretchr/testify from 1.6.1 to 1.7.0 Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.6.1 to 1.7.0. - [Release notes](https://github.com/stretchr/testify/releases) - [Commits](stretchr/testify@v1.6.1...v1.7.0) Signed-off-by: dependabot[bot] <support@github.com> * Auto-fix go.sum changes in dependent modules * Auto-fix go.sum changes in dependent modules Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
This is in support of issue #652.
This change:
correctnesspackage so that they can be used from themetricssubdirectory/package.MockTraceConsumerandMockMetricsConsumerwith the interfacesMetricsDualConsumerandTraceDualConsumer, both of which can consume old and new metrics/traces. This will be used in a forthcomingTestHarnesstype that implementsMetricsDualConsumer.PrometheusDataReceiverand aPrometheusDataSenderso that Prometheus correctness can be tested.