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.

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

Fix metric/spans count, add tests for nil entries in the slices #787

Merged
merged 1 commit into from Apr 7, 2020

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Apr 5, 2020

Fixes in this PR:

  • Handle correctly nil Resource[Spans|Metrics], InstrumentationLibrary[Spans|Metrics], Span|Metric, MetricDataPoints.
  • Found a bug that the OTLP MetricDescriptor.Labels were ignored in Internal->OC
  • Rename translator/internaldata/{oc_testdata.go => oc_testdata_test.go} to not count these lines in coverage.

Work left for slices:

  • Tests and fixes for nil Events, Links, Buckets, ValueAtPercentile

Work left for maps:

  • Define what should be the behavior if a nil entry in the "slice map representation".
  • Add tests for these cases.

@codecov-io
Copy link

codecov-io commented Apr 5, 2020

Codecov Report

Merging #787 into master will decrease coverage by 0.04%.
The diff coverage is 97.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #787      +/-   ##
==========================================
- Coverage   85.54%   85.49%   -0.05%     
==========================================
  Files         154      153       -1     
  Lines       11520    11510      -10     
==========================================
- Hits         9855     9841      -14     
- Misses       1293     1295       +2     
- Partials      372      374       +2     
Impacted Files Coverage Δ
translator/internaldata/metrics_to_oc.go 92.60% <82.85%> (-3.65%) ⬇️
internal/data/metric.go 95.55% <100.00%> (+0.68%) ⬆️
internal/data/testdata/common.go 100.00% <100.00%> (ø)
internal/data/testdata/metric.go 100.00% <100.00%> (ø)
internal/data/testdata/trace.go 100.00% <100.00%> (ø)
internal/data/trace.go 94.87% <100.00%> (+0.93%) ⬆️
translator/internaldata/oc_to_metrics.go 88.54% <100.00%> (+2.90%) ⬆️
translator/internaldata/traces_to_oc.go 80.18% <100.00%> (+0.83%) ⬆️
translator/internaldata/resource_to_oc.go 71.01% <0.00%> (+2.89%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d1a9b7...27e41d7. Read the comment docs.

@bogdandrutu bogdandrutu force-pushed the nilstests branch 2 times, most recently from 89a07e8 to 7062c01 Compare April 5, 2020 20:21
translator/internaldata/metrics_to_oc.go Show resolved Hide resolved
translator/internaldata/metrics_to_oc.go Show resolved Hide resolved
Comment on lines +245 to +260
if metric.Int64DataPoints().Len() != 0 {
metric.Int64DataPoints().Resize(int64DataPointsNum)
}
if metric.DoubleDataPoints().Len() != 0 {
metric.DoubleDataPoints().Resize(doubleDataPointsNum)
}
if metric.HistogramDataPoints().Len() != 0 {
metric.HistogramDataPoints().Resize(histogramDataPointsNum)
}
if metric.SummaryDataPoints().Len() != 0 {
metric.SummaryDataPoints().Resize(summaryDataPointsNum)
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we have this code. Is it to remove nil data points?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, resize does init with empty elements, so otherwise if we skipped any element because of nil we should remove the empty elements from the end of the slice.

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@bogdandrutu
Copy link
Member Author

@tigrannajaryan PTAL

@tigrannajaryan tigrannajaryan merged commit fe9abe7 into open-telemetry:master Apr 7, 2020
@tigrannajaryan
Copy link
Member

@bogdandrutu Please use the PR description that you create as the commit message as well. I typically copy/paste them when merging, but someone may forget when merging. We want nice commit messages.

@flands flands added this to the Beta 0.4 milestone Jun 4, 2020
@bogdandrutu bogdandrutu deleted the nilstests branch August 5, 2020 18:50
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants