-
Notifications
You must be signed in to change notification settings - Fork 835
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: add ConsoleMetricExporter #3120
feat: add ConsoleMetricExporter #3120
Conversation
Allow to export metrics to the console refs open-telemetry#3036
Codecov Report
@@ Coverage Diff @@
## main #3120 +/- ##
==========================================
+ Coverage 93.13% 93.21% +0.08%
==========================================
Files 196 196
Lines 6406 6414 +8
Branches 1350 1350
==========================================
+ Hits 5966 5979 +13
+ Misses 440 435 -5
|
experimental/packages/opentelemetry-sdk-metrics-base/src/export/ConsoleMetricExporter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/ConsoleMetricExporter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/ConsoleMetricExporter.ts
Outdated
Show resolved
Hide resolved
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.
You may need to remove the incomplete one defined in experimental/packages/opentelemetry-sdk-metrics-base/src/export/MetricExporter.ts
and export the new one in experimental/packages/opentelemetry-sdk-metrics-base/src/index.ts
.
Thank you! I have made your requested changes |
experimental/packages/opentelemetry-sdk-metrics-base/src/export/ConsoleMetricExporter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/test/export/ConsoleMetricExporter.test.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/test/export/ConsoleMetricExporter.test.ts
Show resolved
Hide resolved
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.
Looks good overall, just a few nits and comments. 🙂
Thank you for working on this! 🙂
experimental/packages/opentelemetry-sdk-metrics-base/src/export/ConsoleMetricExporter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/export/ConsoleMetricExporter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/test/export/ConsoleMetricExporter.test.ts
Outdated
Show resolved
Hide resolved
@hectorhdzg @pichlermarc I think I have accommodated both your feedback on the PR :) |
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.
LGTM
export(metrics: ResourceMetrics, resultCallback: (result: ExportResult) => void): void { | ||
if (this._shutdown) { | ||
// If the exporter is shutting down, by spec, we need to return FAILED as export result | ||
setImmediate(resultCallback, { code: ExportResultCode.FAILED }); |
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.
Probably good to include a reason
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.
Looks good. Thanks for addressing the comments. 🙂
Which problem is this PR solving?
Adds a new metric exporter
ConsoleMetricExporter
to export or log the metrics to the consoleFixes #3036
Short description of the changes
Adds
ConsoleMetricExporter
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: