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

New stdoutmetric encoder with ignore timestamp #3828

Merged
merged 10 commits into from
Mar 9, 2023

Conversation

Petrie
Copy link
Contributor

@Petrie Petrie commented Mar 2, 2023

Resolve: #3223

Signed-off-by: Peter Liu <lpfvip2008@gmail.com>
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #3828 (11a1346) into main (e463505) will increase coverage by 0.1%.
The diff coverage is 96.2%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3828     +/-   ##
=======================================
+ Coverage   81.6%   81.7%   +0.1%     
=======================================
  Files        169     169             
  Lines      12489   12570     +81     
=======================================
+ Hits       10201   10281     +80     
- Misses      2073    2074      +1     
  Partials     215     215             
Impacted Files Coverage Δ
exporters/stdout/stdoutmetric/encoder.go 100.0% <ø> (ø)
exporters/stdout/stdoutmetric/exporter.go 97.1% <96.0%> (-2.9%) ⬇️
exporters/stdout/stdoutmetric/config.go 75.4% <100.0%> (+2.5%) ⬆️

... and 1 file with indirect coverage changes

@MrAlias
Copy link
Contributor

MrAlias commented Mar 3, 2023

The approach showcased in #3529 added an option instead of a dedicated encoder. That approach would apply to all encoders instead of a single one.

Should that approach be taken instead?

@Petrie
Copy link
Contributor Author

Petrie commented Mar 4, 2023

The approach showcased in #3529 added an option instead of a dedicated encoder. That approach would apply to all encoders instead of a single one.

Should that approach be taken instead?

I apologize for not noticing the "POC" pull request. After reviewing the code structure, I believe it is more organized than mine. I will refactor my code to avoid using a dedicated encoder.

Signed-off-by: Peter Liu <lpfvip2008@gmail.com>
Signed-off-by: Peter Liu <lpfvip2008@gmail.com>
@Petrie Petrie marked this pull request as draft March 4, 2023 22:56
Signed-off-by: Peter Liu <lpfvip2008@gmail.com>
@Petrie Petrie marked this pull request as ready for review March 5, 2023 03:35
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.

Over all looks good. The testing needs some coverage added to it.

exporters/stdout/stdoutmetric/exporter.go Outdated Show resolved Hide resolved
exporters/stdout/stdoutmetric/exporter.go Show resolved Hide resolved
exporters/stdout/stdoutmetric/exporter.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Mar 8, 2023

Please be sure to add a changelog entry.

Petrie and others added 3 commits March 8, 2023 09:33
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Signed-off-by: Peter Liu <lpfvip2008@gmail.com>
@Petrie Petrie requested a review from MrAlias March 9, 2023 05:43
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I have forgotten to click "submit review" a few hours ago 😆

CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias added this to the Metric SDK v0.38.0 milestone Mar 9, 2023
@MrAlias MrAlias merged commit 6a95d57 into open-telemetry:main Mar 9, 2023
@Petrie Petrie deleted the new-stdoutmetric-encoder branch March 9, 2023 22:27
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.

Add JSON encoder to stdoutmetric exporter that ignores timestamps
4 participants