feat(sdk-metrics-base): update metric exporter interfaces #2707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #2707 +/- ##
==========================================
- Coverage 93.42% 93.41% -0.02%
==========================================
Files 159 159
Lines 5449 5450 +1
Branches 1143 1145 +2
==========================================
Hits 5091 5091
- Misses 358 359 +1
|
|
Nit: the prefix of this PR should be |
|
Thanks, @legendecas. I was wondering if this change is needed since you had a question about the naming here #2589 (comment) |
|
@lonewolf3739 I didn't mean we don't need this change. I mean we need a spec clarification on the interface naming. Just opened open-telemetry/opentelemetry-specification#2286. |
srikanthccv
changed the title
tigrannajaryan
pushed a commit
to open-telemetry/opentelemetry-specification
that referenced
this pull request
Feb 15, 2022
If the implementors choose to implement pull metric exporters modeled as a metric reader, the only variant for metric exporter interface is push metric exporter. While the pull metric exporter is modeled as a "metric reader", the better naming for the genric "metric exporter" interface may be "push metric exporter". Just open this PR to see if there are similar naming confusions with other SDK implementors. If so, I'd believe the metric exporter and metric reader section need a re-structure in a follow-up PR since how the pull metric exporter is modeled is important to the naming of the metric exporter interface. Related: - open-telemetry/opentelemetry-js#2707 - open-telemetry/opentelemetry-js#2589 (comment)
legendecas
reviewed
Feb 15, 2022
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricExporter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricExporter.ts
Outdated
Show resolved
Hide resolved
|
@lonewolf3739 thank you for your work on this! open-telemetry/opentelemetry-specification#2286 has been landed, I think it is good now to continue with this work. |
vmarchaud
approved these changes
Feb 19, 2022
...rimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts
Outdated
Show resolved
Hide resolved
legendecas
reviewed
Feb 21, 2022
...rimental/packages/opentelemetry-sdk-metrics-base/src/export/PeriodicExportingMetricReader.ts
Outdated
Show resolved
Hide resolved
legendecas
approved these changes
Feb 22, 2022
experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricExporter.ts
Outdated
Show resolved
Hide resolved
…t/MetricExporter.ts Co-authored-by: legendecas <[email protected]>
ChengJinbao
added a commit
to ChengJinbao/opentelemetry-specification
that referenced
this pull request
Nov 16, 2022
If the implementors choose to implement pull metric exporters modeled as a metric reader, the only variant for metric exporter interface is push metric exporter. While the pull metric exporter is modeled as a "metric reader", the better naming for the genric "metric exporter" interface may be "push metric exporter". Just open this PR to see if there are similar naming confusions with other SDK implementors. If so, I'd believe the metric exporter and metric reader section need a re-structure in a follow-up PR since how the pull metric exporter is modeled is important to the naming of the metric exporter interface. Related: - open-telemetry/opentelemetry-js#2707 - open-telemetry/opentelemetry-js#2589 (comment)
joaopgrassi
pushed a commit
to dynatrace-oss-contrib/semantic-conventions
that referenced
this pull request
Mar 21, 2024
If the implementors choose to implement pull metric exporters modeled as a metric reader, the only variant for metric exporter interface is push metric exporter. While the pull metric exporter is modeled as a "metric reader", the better naming for the genric "metric exporter" interface may be "push metric exporter". Just open this PR to see if there are similar naming confusions with other SDK implementors. If so, I'd believe the metric exporter and metric reader section need a re-structure in a follow-up PR since how the pull metric exporter is modeled is important to the naming of the metric exporter interface. Related: - open-telemetry/opentelemetry-js#2707 - open-telemetry/opentelemetry-js#2589 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #2589
This PR updates the exporter interfaces. Renames the push exporter type to
PushMetricExporterto make it more clear. Makes the shutdown abstract so that implementation can have control over things such as closing connections etc, which one might not do inforceFlush. Andexportreturns promise withExportResult.