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

Jump to bottom

Improve metric test helpers for instrumentation #872

Closed
jmacd opened this issue Jun 25, 2020 · 6 comments · Fixed by #1040 or #2105
Closed

Improve metric test helpers for instrumentation #872

jmacd opened this issue Jun 25, 2020 · 6 comments · Fixed by #1040 or #2105
Assignees
@jmacd
Labels
area:metrics Part of OpenTelemetry Metrics pkg:testing Related to testing or a testing package
Milestone
RC1

Comments

@jmacd
Copy link
Contributor

jmacd commented Jun 25, 2020

There is a test helper in internal/metric used by a few of the tests in this repository. They're not great tests from a readability standpoint, but this is the only facility provided to test instrumentation with a mock Meter.

There is a better test helper used internally in a number of tests, sdk/metric/processor/test.Output, which makes for reasonably concise and readable tests. Something similar should be provided for testing metrics, probably outside of the internal directory so that it can be used for testing external code.

As part of this ticket, improve the testing in instrumentation/othttp/handler_test.go as discussed here.
@jmacd jmacd added the area:metrics Part of OpenTelemetry Metrics label Jun 25, 2020
@jmacd jmacd mentioned this issue Jun 25, 2020
Merged
@AndrewGrachov
Copy link
Contributor

Will take it

@AndrewGrachov
Copy link
Contributor

AndrewGrachov commented Jun 29, 2020 •
edited
Loading

Found quite nice test implemetations here, so it could be just moving things around (to make this hierarchical correct) and small tests refactor to do proper checks

https://github.com/open-telemetry/opentelemetry-go/blob/master/api/global/internal/meter_test.go#L116

On the other hand we could instrument mockMeter with Output, but the more I dig, the more it seems to be unnecessary

What you would say @jmacd @Aneurysm9 ?

@MrAlias MrAlias added the pkg:testing Related to testing or a testing package label Jul 2, 2020
This was referenced Jul 29, 2020
Closed
Merged
@Aneurysm9 Aneurysm9 mentioned this issue Aug 5, 2020
Merged
@MrAlias MrAlias mentioned this issue Aug 5, 2020
Closed
@MrAlias
Copy link
Contributor

MrAlias commented Aug 5, 2020

@AndrewGrachov are you still working on this?

@AndrewGrachov
Copy link
Contributor

Got stuck.

Really dont see a good way to implement it via sdk/test.Output - its heavily dependent on counter names, e.g. using suffixes to do aggregation checks, which irrelevant when testing instrumentation

So I guess we could just expose test helpers from mock meter, which do assertion, and re-use them.

Will send a PR today/tomorrow, for that I have clear view

@AndrewGrachov
Copy link
Contributor

left a concept here

https://github.com/open-telemetry/opentelemetry-go/pull/1040/files

@MrAlias MrAlias added this to the RC1 milestone Aug 27, 2020
@MrAlias MrAlias linked a pull request Aug 27, 2020 that will close this issue
#872 Make metric test helpers public #1040
Merged
MrAlias added a commit that referenced this issue Aug 27, 2020
@AndrewGrachov @MrAlias
#872 Make metric test helpers public (#1040)
1c3626e 
* Make metric test helpers public

* Move everything metric test related to api/metric/metrictest

* Unify metric measurement assertions

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
evantorrie pushed a commit to evantorrie/opentelemetry-go that referenced this issue Sep 10, 2020
@AndrewGrachov @MrAlias @evantorrie
open-telemetry#872 Make metric test helpers public (open-telemetry#1040)
12769f5 
* Make metric test helpers public

* Move everything metric test related to api/metric/metrictest

* Unify metric measurement assertions

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@jmacd jmacd mentioned this issue Jul 20, 2021
Merged
@jmacd
Copy link
Contributor Author

jmacd commented Jul 20, 2021

Re-opening this issue for its original purpose, to remove the two TODOs marked #872.

@jmacd jmacd reopened this Jul 20, 2021
@MrAlias MrAlias assigned jmacd and unassigned AndrewGrachov Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jmacd jmacd

Labels
area:metrics Part of OpenTelemetry Metrics pkg:testing Related to testing or a testing package
Projects
None yet
Milestone
RC1
3 participants
@AndrewGrachov @jmacd @MrAlias