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

[pkg/ottl] add MarshalLogObject to ottlmetric context #38293

Merged
merged 6 commits into from
Mar 4, 2025

Conversation

odubajDT
Copy link
Contributor

@odubajDT odubajDT commented Mar 3, 2025

Link to tracking issue

Fixes #38103

@odubajDT odubajDT marked this pull request as ready for review March 3, 2025 08:12
@odubajDT odubajDT requested a review from a team as a code owner March 3, 2025 08:12
@odubajDT odubajDT changed the title [pkg/ottl] add MarshalLogObject() for ottlmetric context [pkg/ottl] add MarshalLogObject for ottlmetric context Mar 3, 2025
@odubajDT odubajDT changed the title [pkg/ottl] add MarshalLogObject for ottlmetric context [pkg/ottl] add MarshalLogObject to ottlmetric context Mar 3, 2025
@odubajDT odubajDT force-pushed the ottl-marshall-logobject branch from 2faac0b to 9c0f794 Compare March 3, 2025 12:18
@odubajDT odubajDT requested a review from edmocosta March 3, 2025 12:42
@@ -191,6 +191,18 @@ func (m Metric) MarshalLogObject(encoder zapcore.ObjectEncoder) error {
return err
}

type MetricSlice pmetric.MetricSlice
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can revert this change as we don't need it anymore.

Copy link
Contributor Author

@odubajDT odubajDT Mar 3, 2025

Choose a reason for hiding this comment

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

Shouldn't we keep it just for potential future use? It does not hurt to have an interface implemented for the MetricSlice, even if it's not used now, things can change in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an internal package, and IMO, if we don't have any usages/plans for that implementation at the moment, we should just drop it, and add it when/if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then, removed

odubajDT and others added 4 commits March 3, 2025 16:19
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Co-authored-by: Edmo Vamerlatti Costa <11836452+edmocosta@users.noreply.github.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
@odubajDT odubajDT force-pushed the ottl-marshall-logobject branch from 71aaac1 to 11abbd5 Compare March 3, 2025 15:21
@odubajDT odubajDT requested a review from edmocosta March 3, 2025 15:22
@edmocosta
Copy link
Contributor

For reference, here's an example of a debug log before and after this change:

2025-03-03T16:51:53.566+0100	debug	ottl@parser.go:356	initial TransformContext before executing StatementSequence	{"TransformContext": {}}
2025-03-03T16:48:14.311+0100	debug	ottl/parser.go:356	initial TransformContext before executing 
StatementSequence	{"TransformContext": {"resource": {"attributes": {"service.name": "my.service", "timestamp": 
"2018-12-01T16:17:18Z"}, "dropped_attribute_count": 0}, "scope": {"attributes": {"my.scope.attribute": "some scope 
attribute"}, "dropped_attribute_count": 0, "name": "my.library", "version": "1.0.0"}, "metric": {"description": "I am a 
Histogram", "name": "my.histogram", "unit": "1", "type": "Histogram", "aggregation_temporality": "Delta", "datapoints": 
[{"attributes": {"my.histogram.attr": "some value"}, "bucket_counts": [1, 1], "count": 2, "exemplars": [], "explicit_bounds": 
[1], "flags": 0, "max": 2, "min": 0, "start_time_unix_nano": 1544712660300000000, "sum": 2, "time_unix_nano": 
1544712660300000000}]}, "cache": {}}}

Copy link
Contributor

@edmocosta edmocosta left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for implementing it!

@TylerHelmuth TylerHelmuth added the ready to merge Code review completed; ready to merge by maintainers label Mar 3, 2025
Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

need to rebase / merge main

@songy23 songy23 removed the ready to merge Code review completed; ready to merge by maintainers label Mar 3, 2025
@evan-bradley evan-bradley merged commit d678dd7 into open-telemetry:main Mar 4, 2025
156 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pkg/ottl] Implement MarshalLogObject for the ottlmetric context
5 participants