Skip to content

Conversation

@fraillt
Copy link
Contributor

@fraillt fraillt commented Dec 11, 2024

Changes

Update CHANGELOG in sdk and stdout crates, to reflect breaking changes made on metric aggregations.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@fraillt fraillt requested a review from a team as a code owner December 11, 2024 06:13
@fraillt
Copy link
Contributor Author

fraillt commented Dec 11, 2024

It's worth noting, that currently #2411 is not closed yet, even though it's mentioned in CHANGELOG already.

@codecov
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.3%. Comparing base (d0ef365) to head (5235bcc).
Report is 1 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2412   +/-   ##
=====================================
  Coverage   79.3%   79.3%           
=====================================
  Files        122     122           
  Lines      21569   21569           
=====================================
  Hits       17114   17114           
  Misses      4455    4455           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


## vNext

- *Breaking* time fields, `StartTime` and `EndTime` is printed on aggregation (Sum, Gauge, Histogram, ExpoHistogram) with 2 tabs, previously it was on aggregation data point, with 3 tabs, see [#2377](https://github.com/open-telemetry/opentelemetry-rust/pull/2377) and [#2411](https://github.com/open-telemetry/opentelemetry-rust/pull/2411).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay to skipp changelog for stdout

Copy link
Contributor Author

@fraillt fraillt Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to mention this... I mean, my company use similar approach in production (have custom exporter that exports metrics as logs using log instead println). So I imagine that someone might rely on the format of stdout exporter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I imagine that someone might rely on the format of stdout exporter.

I don't think so. It's explicitly called out that stdout is only for learning purposes, and its output format should not be relied upon for anything.

https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-stdout/src/lib.rs#L1-L4

(Not a blocker for this PR, just sharing that stdout is not meant for that purpose)

@cijothomas cijothomas merged commit dcaff0d into open-telemetry:main Dec 12, 2024
23 checks passed
@fraillt fraillt deleted the update-change-log-for-aggregation-time-changes branch December 13, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants