-
Notifications
You must be signed in to change notification settings - Fork 805
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(api): added synchronous gauge #4528
feat(api): added synchronous gauge #4528
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4528 +/- ##
==========================================
+ Coverage 90.77% 92.84% +2.07%
==========================================
Files 90 328 +238
Lines 1930 9524 +7594
Branches 417 2050 +1633
==========================================
+ Hits 1752 8843 +7091
- Misses 178 681 +503
|
1ab8704
to
974bf61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clintonb looks awesome already. 🙂
I see this is currently a draft: when getting ready for the review, would you mind splitting the PR into two parts? So in essence:
- PR#1 SDK changes only (merged first)
- PR#2 API changes only (merged second)
The reason for that is: the SDK changes need to be functional with an older version of the API that does not include the new types. If we split that into two PRs we'll very easily be able to see if there's any adjustments that need to be made to the SDK part to ensure that it also works with the older (current) version of the API. That usually includes duplicating types from the API PR#2 into the SDK PR#1 to ensure we don't use anything that might not yet be there 🙂
cbdeadf
to
0bafd46
Compare
ea980b5
to
d339cf4
Compare
e803423
to
313ce47
Compare
@pichlermarc let me know if there is anything I need to do here and/or next steps., |
Sorry this is taking so long. Turns out we need to figure out a way to add experimental metrics features to the API package as we've never done it before with new Instruments. I've opened a prototype PR to look into that #4622. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification was marked stable, so we can proceed with this PR without having to go the experimental route (#4622)
While reviewing this PR I just saw that we put the Type-Parameters on the SDK implementation of the Gauge too:
opentelemetry-js/packages/sdk-metrics/src/Meter.ts
Lines 56 to 59 in 928796d
createGauge<AttributesTypes extends Attributes = Attributes>( | |
name: string, | |
options?: MetricOptions | |
): Gauge<AttributesTypes> { |
These can be removed now as they're part of the interface we're implementing (like on the rest of the functions on the Meter class).
You may now also remove the @experimental
annotations from the SDK that we added in #4565:
* @experimental * @experimental
313ce47
to
0b02976
Compare
@pichlermarc please review. 🤞🏾 |
0b02976
to
2ff80f5
Compare
@pichlermarc one more look? |
Failing test is unrelated to this change. |
@@ -36,7 +37,6 @@ import { | |||
AsyncWritableMetricStorage, | |||
WritableMetricStorage, | |||
} from './state/WritableMetricStorage'; | |||
import { Gauge } from './types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep using Gauge
from ./types
as it could still happen that (for instance) @opentelemetry/api@1.3.0
is used which does not have the Gauge
type.
packages/sdk-metrics/src/Meter.ts
Outdated
@@ -41,7 +41,6 @@ import { | |||
UpDownCounterInstrument, | |||
} from './Instruments'; | |||
import { MeterSharedState } from './state/MeterSharedState'; | |||
import { Gauge } from './types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should keep using Gauge
from ./types
as it could still happen that (for instance) @opentelemetry/api@1.3.0
is used which does not have the Gauge
type.
2ff80f5
to
bead4cf
Compare
@pichlermarc fixes implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 🙂
bead4cf
to
5da7a9d
Compare
@pichlermarc what are the next steps? Do you merge, or someone else? |
* feat(instrumentation): added synchronous gauge * fixup! feat(instrumentation): added synchronous gauge * fixup! feat(instrumentation): added synchronous gauge * fixup! feat(instrumentation): added synchronous gauge
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 #4296
Short description of the changes
Type of change
Please delete options that are not relevant.
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
Checklist: