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

Add test units of histogram instrument kind #2507

Merged
merged 10 commits into from Mar 23, 2022

Conversation

Jacob953
Copy link
Contributor

@Jacob953 Jacob953 commented Jan 12, 2022

Done

  • Add test units of histogram instrument kinds with "FloatmgMetricGroupingExport".

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 12, 2022

CLA Signed

The committers are authorized under a signed CLA.

@XSAM XSAM added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jan 12, 2022
@hanyuancheung
Copy link
Member

Thanks for your contribution!

@MrAlias MrAlias added area:metrics Part of OpenTelemetry Metrics pkg:testing Related to testing or a testing package labels Jan 12, 2022
@Jacob953
Copy link
Contributor Author

Thanks for your contribution!

It would be my pleasure! 🤗

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

I am confused. The original comment of #2442 is to fix the end-to-end test of int64-histogram and float64-histogram, but I do not see such fix in this PR.

https://github.com/hanyuancheung/opentelemetry-go/blob/4d9d882c382641e5f11e72aa9ac9603281fb5a4b/exporters/otlp/otlpmetric/internal/otlpmetrictest/otlptest.go#L57-L58

@Jacob953
Copy link
Contributor Author

I don't think now is the time to fix these two lines since it didn't support Summary Metrics well for now.

Intentionally trigger the assertion by making this line wrong to get objects of expected data back from exp.

[name:"test-int64-counter" sum:{data_points:{attributes:{key:"test" value:{bool_value:true}} start_time_unix_nano:1644882047272998000 time_unix_nano:1644882047273078000 as_int:1} aggregation_temporality:AGGREGATION_TEMPORALITY_CUMULATIVE is_monotonic:true}
name:"test-float64-counter" sum:{data_points:{attributes:{key:"test" value:{bool_value:true}} start_time_unix_nano:1644882047272998000 time_unix_nano:1644882047273078000 as_double:1} aggregation_temporality:AGGREGATION_TEMPORALITY_CUMULATIVE is_monotonic:true}
name:"test-int64-histogram" sum:{data_points:{attributes:{key:"test" value:{bool_value:true}} start_time_unix_nano:1644882047272998000 time_unix_nano:1644882047273078000 as_int:2} aggregation_temporality:AGGREGATION_TEMPORALITY_CUMULATIVE}
name:"test-float64-histogram" sum:{data_points:{attributes:{key:"test" value:{bool_value:true}} start_time_unix_nano:1644882047272998000 time_unix_nano:1644882047273078000 as_double:2} aggregation_temporality:AGGREGATION_TEMPORALITY_CUMULATIVE}
name:"test-int64-gaugeobserver" gauge:{data_points:{attributes:{key:"test" value:{bool_value:true}} time_unix_nano:1644882047273073000 as_int:3}}
name:"test-float64-gaugeobserver" gauge:{data_points:{attributes:{key:"test" value:{bool_value:true}} time_unix_nano:1644882047273074000 as_double:3}}]

The assertion obviously points that the object of Histogram is Sum which the same as Counter rather than Summary.

I think there is nothing wrong with this test code file by checking all relevant test files :)

@Jacob953
Copy link
Contributor Author

And I'll try my best to take a part in #2280

@Jacob953 Jacob953 requested a review from XSAM February 28, 2022 03:24
@XSAM
Copy link
Member

XSAM commented Feb 28, 2022

I accidentally deleted one of my comments. Here is the re-cap.

I think there is nothing wrong with this test code file by checking all relevant test files :)

Yeah, I think this PR is good. But it has nothing related to #2442. Would you like to delete the link with #2442?

@hanyuancheung
Copy link
Member

I agree. Could you please open another pull request to resolve the ploblem above? @Jacob953

@Jacob953 Jacob953 changed the title fix histogram instrument unit test Add test units of histogram instrument kind Feb 28, 2022
@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

Merging #2507 (7713723) into main (07ad32d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2507   +/-   ##
=====================================
  Coverage   76.7%   76.7%           
=====================================
  Files        181     181           
  Lines      12175   12175           
=====================================
  Hits        9345    9345           
  Misses      2605    2605           
  Partials     225     225           
Impacted Files Coverage Δ
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-1.0%) ⬇️
exporters/jaeger/jaeger.go 91.1% <0.0%> (+0.8%) ⬆️

@Jacob953
Copy link
Contributor Author

I agree. Could you please open another pull request to resolve the problem above? @Jacob953

I'll always pay attention to this problem. And could this PR be merged? :)

@MrAlias MrAlias closed this Mar 23, 2022
@MrAlias MrAlias reopened this Mar 23, 2022
@MrAlias MrAlias merged commit 3dac50a into open-telemetry:main Mar 23, 2022
@MrAlias MrAlias added this to the Release v1.6.0 milestone Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:testing Related to testing or a testing package Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants