8000 sdk/metric/metricdata: Add MarshalJSON for Extrema by m-posluszny · Pull Request #4827 · open-telemetry/opentelemetry-go · GitHub
[go: up one dir, main page]
Skip to content

sdk/metric/metricdata: Add MarshalJSON for Extrema #4827

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Feb 1, 2024
Merged

sdk/metric/metricdata: Add MarshalJSON for Extrema #4827

merged 14 commits into from
Feb 1, 2024

Conversation

m-posluszny
Copy link
Contributor

Fixes #4826

Issue

Json representation was missing Max and Min values, due to private fields of Extrema type.

[{"Attributes":[],"StartTime":"2024-01-14T13:09:48.50817+08:00","Time":"2024-01-14T13:10:48.506979+08:00","Count":4,"Bounds":[0,5,10,25,50,75,100,250,500,750,1000,2500,5000,7500,10000],"BucketCounts":[0,0,0,0,0,0,0,3,0,0,1,0,0,0,0,0],"Min":{},"Max":{},"Sum":1452}],"Temporality":"DeltaTemporality"}}]

Solution

Implemented MarshalJSON method, that converts field value of Extrema to BJSON number type. In case of invalid number, 0 will be returned.

Does it work?

I've tested it over example provided in mentioned issue. I've managed to get min and max values of time durations. Moreover I've added unit tests to cover json marshalling for Extrema type.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link
linux-foundation-easycla bot commented Jan 15, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

MrAlias
MrAlias reviewed Jan 15, 2024
View reviewed changes
Copy link
Contributor
@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Please sign the CLA so these changes can be considered for merge.

pellared
pellared reviewed Jan 16, 2024
View reviewed changes
< 8000 div data-show-on-forbidden-error hidden>

Uh oh!

There was an error while loading. Please reload this page.

@pellared pellared changed the title Implement MarshalJSON for Extrema [Issue #4826] Add MarshalJSON for Extrema Jan 16, 2024
@pellared pellared changed the title Add MarshalJSON for Extrema sdk/metric/metricdata: Add MarshalJSON for Extrema Jan 16, 2024
@pellared
Copy link
Member

Can you add a changelog entry?

pellared
pellared reviewed Jan 18, 2024
View reviewed changes
pellared
pellared reviewed Jan 18, 2024
View reviewed changes
pellared
pellared reviewed Jan 18, 2024
View reviewed changes
@pellared
Copy link
Member

Unit tests in stdoutmetric need to be fixed/updated.

@m-posluszny
Copy link
Contributor Author

Sorry for the force push, had to reverse problematic rebase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pellared pellared pellared approved these changes

@MrAlias MrAlias MrAlias approved these changes

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@hanyuancheung hanyuancheung Awaiting requested review from hanyuancheung

@dmathieu dmathieu Awaiting requested review from dmathieu dmathieu is a code owner

+1 more reviewer

@MadVikingGod MadVikingGod MadVikingGod approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.23.0
Development

Successfully merging this pull request may close these issues.

I cannot Get Max/Min Value In Histogram type
4 participants
@m-posluszny @pellared @MadVikingGod @MrAlias
0