Skip to content

[shipping] Use cumulative metrics in shipping service to be consistent with the other services of the demo#2503

Merged
julianocosta89 merged 4 commits intoopen-telemetry:mainfrom
cyrille-leclerc:use-delta-temporality-on-shipping
Sep 5, 2025
Merged

[shipping] Use cumulative metrics in shipping service to be consistent with the other services of the demo#2503
julianocosta89 merged 4 commits intoopen-telemetry:mainfrom
cyrille-leclerc:use-delta-temporality-on-shipping

Conversation

@cyrille-leclerc
Copy link
Copy Markdown
Member

@cyrille-leclerc cyrille-leclerc commented Sep 3, 2025

Changes

Use cumulative metrics in shipping service to be consistent with the other services of the demo.

Note that Prometheus can't ingest delta metrics and we have an exception in the collector when using delta metrics.

I checked, metrics produced by the shipping metrics like http.server.duration or app.shipping.items_count work well in PRometheus when produced as cumulative metrics.

Merge Requirements

For new features contributions, please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@cyrille-leclerc cyrille-leclerc marked this pull request as ready for review September 3, 2025 21:17
@cyrille-leclerc cyrille-leclerc requested a review from a team as a code owner September 3, 2025 21:17
@cyrille-leclerc cyrille-leclerc changed the title Use cumulative metrics in shipping service to be consistent with the other services of the demo [shipping] Use cumulative metrics in shipping service to be consistent with the other services of the demo Sep 3, 2025
@cyrille-leclerc
Copy link
Copy Markdown
Member Author

cyrille-leclerc commented Sep 4, 2025

@julianocosta89 I think that you are the author of the piece of code that specify to use Delta temporality in the Shipping service instead of the default cumulative temporality. Do you remember the rationale?
I don't see problems testing with cumulative, is there a challenge somewhere?

See commit diff 00142af#diff-6e0dcf073aecdc8a72dcc3dc9264e65fe0b91c859b4017bde948bf48e90a72fdR45

@julianocosta89
Copy link
Copy Markdown
Member

hey @cyrille-leclerc thanks for that!
The rationale is that I simply forgot about it 😅 haha.

The temporality should be configured via env var and not via code.
I'll add to your PR the OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE on the shipping service.

Thanks for raising this

@github-actions github-actions bot added the helm-update-required Requires an update to the Helm chart when released label Sep 5, 2025
@cyrille-leclerc
Copy link
Copy Markdown
Member Author

Thanks @julianocosta89,
FYI I didn't mention it in this PR but I'm looking with @ywwg at making it easy to switch the entire OTel Demo from cumulative to delta in order to test the ongoing work of the Prometheus community to support delta temporality. We will be happy to pair with you on this topic as we see this is also in your radar.
An item I have in mind for example is to switch OTel Collector infrastructure monitoring receivers like HostMetrics or PostgreSQL receivers to delta temporality, I don't know if the OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE env var would work for this.

@julianocosta89
Copy link
Copy Markdown
Member

@cyrille-leclerc the OTEL_EXPORTER_OTLP_METRICS_TEMPORALITY_PREFERENCE should do what you are trying to achieve.

The whole demo should be able to be configured just with this env var to switch from cumulative to delta.

@julianocosta89 julianocosta89 merged commit 4d64f2d into open-telemetry:main Sep 5, 2025
31 checks passed
@cyrille-leclerc cyrille-leclerc deleted the use-delta-temporality-on-shipping branch September 5, 2025 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

helm-update-required Requires an update to the Helm chart when released

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants