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

NH-91603: custom otlp metrics through otlp protocol #169

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Nov 25, 2024

Description

  1. add custom metrics with otlp protocol
  2. add metrics-sdk as one of the dependencies

Current limitation (need more clarification):

  1. user have to install metrics exporter
  2. user have to decide when the metrics exporter will happen (either through periodic reader or manual)

Test (if applicable)

@xuan-cao-swi xuan-cao-swi changed the title Nh 91603 NH-91603: custom otlp metrics through otlp protocol Nov 25, 2024
@xuan-cao-swi xuan-cao-swi marked this pull request as ready for review November 25, 2024 19:05
@xuan-cao-swi xuan-cao-swi requested a review from a team as a code owner November 25, 2024 19:05
@cheempz
Copy link
Contributor

cheempz commented Nov 26, 2024

Thanks for noting the limitation/question @xuan-cao-swi! I'd like our custom distro to do both: take a dependency on the exporter so that it is installed by default (guess it's not yet part of opentelemetry-sdk and eventually will be?), and to set up a periodic reader.

Any thoughts on possible downside / risk if we went this way?

@xuan-cao-swi
Copy link
Contributor Author

I'd like our custom distro to do both: take a dependency on the exporter so that it is installed by default (guess it's not yet part of opentelemetry-sdk and eventually will be?)

Also, otlp trace exporter is not part of opentelemetry-ruby sdk. I don't think they will include any exporter in sdk; but metrics-sdk will be part of sdk eventually.

We install opentelemetry-exporter-otlp and opentelemetry-exporter-otlp-metrics by default in lambda through layer build, but not in our agent gemspec.

To have periodic reader by default, then we may discourage user to manually pull/export the metrics. If user use aggregation_temporality as :delta, then user may experience strange metrics overtime if they use periodic reader and manual export together.

@xuan-cao-swi
Copy link
Contributor Author

Actually the PeriodicMetricReader is enabled by default: https://github.com/open-telemetry/opentelemetry-ruby/blob/opentelemetry-metrics-sdk/v0.4.0/metrics_sdk/lib/opentelemetry/sdk/metrics/configuration_patch.rb#L44

User have choice to decide the duration of exporting

@cheempz
Copy link
Contributor

cheempz commented Nov 26, 2024

We install opentelemetry-exporter-otlp and opentelemetry-exporter-otlp-metrics by default in lambda through layer build, but not in our agent gemspec.

Gotcha, let's make the same change in our custom distro, which also means no need to special case this in the lambda layer build :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants