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

feat(opentelemetry-api-metrics): Adding generics to create{metricType} #3170

Merged
merged 2 commits into from
Sep 12, 2022
Merged

feat(opentelemetry-api-metrics): Adding generics to create{metricType} #3170

merged 2 commits into from
Sep 12, 2022

Conversation

tomerghelber-tm
Copy link
Contributor

Which problem is this PR solving?

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #3151

Short description of the changes

Added generic to create{metricType} methods a the types or the metrics.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • When creating a metric by create{metricType}, it doesn't require labels as a must.
  • When creating a metric bycreate{metricType} with labels, the labels are a must.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@tomerghelber-tm tomerghelber-tm requested a review from a team August 16, 2022 07:32
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 16, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: tomerghelber-tm / name: Tomer Ghelber (3b1f704)

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

@tomerghelber-tm
Copy link
Contributor Author

tomerghelber-tm commented Aug 17, 2022

Thank you for working on this!

Would you mind adding type conformance tests like https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-base/test/common/Sampler.test.ts#L39?

Do you mean to check every implementation of every Metric type (Counter, Histogram, etc ...)?

The error should be in "compile time", so I am not sure how to test it.

I understand that the example test checks the values are the expected type, no?

@dyladan
Copy link
Member

dyladan commented Aug 17, 2022

The error should be in "compile time", so I am not sure how to test it.

You can use the @ts-expect-errror feature of the ts compiler to test this. The compiler will fail if there is not an error if this comment is used.

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #3170 (53743fa) into main (b73c29d) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3170   +/-   ##
=======================================
  Coverage   93.30%   93.30%           
=======================================
  Files         203      203           
  Lines        6606     6606           
  Branches     1389     1389           
=======================================
  Hits         6164     6164           
  Misses        442      442           
Impacted Files Coverage Δ
...ages/opentelemetry-api-metrics/src/types/Metric.ts 100.00% <ø> (ø)

@dyladan
Copy link
Member

dyladan commented Aug 23, 2022

Any update on @legendecas's comments here?

@tomerghelber-tm
Copy link
Contributor Author

Removed the NOOP generic and added the tests.

@tomerghelber-tm tomerghelber-tm requested review from legendecas and dyladan and removed request for legendecas and dyladan September 4, 2022 16:24
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thank you for working on this. Just a few nits.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

@legendecas
Copy link
Member

Linter is complaining. A format is required.

@tomerghelber-tm tomerghelber-tm requested review from legendecas and dyladan and removed request for legendecas September 6, 2022 09:13
@legendecas legendecas added this to the Metrics GA milestone Sep 7, 2022
@legendecas legendecas added the api:metrics Issues and PRs related to the Metrics API label Sep 7, 2022
@dyladan dyladan removed this from the Metrics GA milestone Sep 7, 2022
@dyladan
Copy link
Member

dyladan commented Sep 12, 2022

Re-ran tests. Flaky test should be fixed by https://github.com/open-telemetry/opentelemetry-js/pull/3242/files

@dyladan dyladan merged commit f9f7fed into open-telemetry:main Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api:metrics Issues and PRs related to the Metrics API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics should enforce labels by typing
3 participants