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

update oc internal, prometheus exporters to use attributes #3770

Merged

Conversation

codeboten
Copy link
Contributor

Description: Started moving code from labels to attributes. I initially started with the ocinternal and then ended up having to make a few more changes in order to get all the tests passing. If this change is too big to be reviewed I'm happy to refactor.

Link to tracking Issue: Part of #3535

Testing: Updated all the existing tests

Followup to #3761

@codeboten codeboten force-pushed the codeboten/0.9.0-attributes-2 branch from 74114dd to 13ead4d Compare August 3, 2021 23:19
@bogdandrutu
Copy link
Member

Needs a rebase

@codeboten codeboten force-pushed the codeboten/0.9.0-attributes-2 branch from 13ead4d to 72e57f3 Compare August 9, 2021 15:09
@codeboten codeboten marked this pull request as ready for review August 9, 2021 15:09
@codeboten codeboten requested review from alolita, Aneurysm9 and a team as code owners August 9, 2021 15:09
@codeboten codeboten closed this Aug 9, 2021
@codeboten codeboten reopened this Aug 9, 2021
l[key] = prompb.Label{
Name: sanitize(key),
Value: value,
Value: value.StringVal(), // TODO(codeboten): Decide what to do with non-string attributes.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably stringify non-string attributes. Unclear what encoding to use though for non-primitive types. Perhaps json-string or something like that since that's also what Otel spec recommends for exporting array attributes when arrays are not natively supported in the target format: https://github.com/open-telemetry/opentelemetry-specification/blob/7cef355a15c4430aa323d155206a2945fcd67a05/specification/common/common.md#attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure i can open a separate issue to track that work

Copy link
Member

Choose a reason for hiding this comment

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

Please open the issue to not forget :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done #3815

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM

exporter/exporterhelper/resource_to_label.go Outdated Show resolved Hide resolved
exporter/exporterhelper/resource_to_label.go Show resolved Hide resolved
exporter/prometheusexporter/accumulator.go Outdated Show resolved Hide resolved
translator/internaldata/metrics_to_oc.go Outdated Show resolved Hide resolved
@codeboten codeboten closed this Aug 11, 2021
@codeboten codeboten reopened this Aug 11, 2021
@bogdandrutu bogdandrutu merged commit 632ed32 into open-telemetry:main Aug 11, 2021
@codeboten codeboten deleted the codeboten/0.9.0-attributes-2 branch June 22, 2023 15:17
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.

None yet

4 participants