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.

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

setting TraceID and SpanID fields in Exemplar objects in spanmetricsp… #13401

Merged
merged 1 commit into from
Oct 13, 2022
Merged

setting TraceID and SpanID fields in Exemplar objects in spanmetricsp… #13401

merged 1 commit into from
Oct 13, 2022

Conversation

arun-shopify
Copy link
Contributor

@arun-shopify arun-shopify commented Aug 17, 2022 •
edited
Loading

Currently TraceID and SpanID fields of Examplar are set in FilteredAttributes (here) in spanmetricsprocessor and passed around but these fields are not part of FilteredAttributes as per the data model: https://pkg.go.dev/go.opentelemetry.io/collector/pdata/internal/data/protogen/metrics/v1#Exemplar

  • This PR sets TraceID and SpanID fields in Exemplar and also adds SpanID to spanmetricsprocessor
  • prometheusexporter is updated to use TraceID, SpanID fields in Exemplar.

Testing:
Unit tests are updated to test TraceID and SpanID fields instead of checking for those in FilteredAttributes

@arun-shopify arun-shopify marked this pull request as ready for review August 17, 2022 18:02
@arun-shopify arun-shopify requested a review from a team as a code owner August 17, 2022 18:02
@arun-shopify arun-shopify mentioned this pull request Aug 17, 2022
Merged
@dashpole dashpole added processor/spanmetrics Span Metrics processor bug Something isn't working labels Aug 17, 2022
@arun-shopify
Copy link
Contributor Author

Do we need to create a new issue for this so that it can be added to unreleased yaml? I couldn't find an existing issue that states this exact problem but there are a few exemplar related issues that are generally related, not sure if I could use on of those. Or would this be a "chore" item that can skip the change log check.

@bogdandrutu
Copy link
Member

If no issue, you can use the PR number as the issue number in the unreleased template.

@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from 31a6c90 to 925b749 Compare August 23, 2022 21:09
@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch 2 times, most recently from 91e0ce8 to b49111d Compare August 25, 2022 13:17
@arun-shopify
Copy link
Contributor Author

@bogdandrutu pinging for review :)

@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch 3 times, most recently from 826fa33 to d49c57f Compare September 6, 2022 20:41
@arun-shopify
Copy link
Contributor Author

@bogdandrutu pinging for review :)

@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch 2 times, most recently from c83be56 to 9754b05 Compare September 9, 2022 20:12
@bogdandrutu
Copy link
Member

I am not one of the owners of the spanmetricsprocessor. @albertteoh please review

albertteoh
albertteoh approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@albertteoh albertteoh left a comment

Choose a reason for hiding this comment

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

The spanmetrics processor aspect of this PR looks good to me, thanks for the fix @arun-shopify!

@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from 9754b05 to bf2ed93 Compare September 16, 2022 16:07
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2022

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Oct 1, 2022
@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from bf2ed93 to 6729fdd Compare October 5, 2022 20:59
@arun-shopify
Copy link
Contributor Author

arun-shopify commented Oct 5, 2022 •
edited
Loading

@bogdandrutu, @albertteoh, pinging for merging if things looks okay.

@github-actions github-actions bot removed the Stale label Oct 6, 2022
@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from 6729fdd to ff800e5 Compare October 6, 2022 13:46
@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from ff800e5 to 5c38543 Compare October 6, 2022 14:21
@arun-shopify
Copy link
Contributor Author

@Aneurysm9 @djaglowski pinging for review :)

@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from 5c38543 to 9f93fa0 Compare October 6, 2022 17:09
@albertteoh
Copy link
Contributor

@bogdandrutu, @albertteoh, pinging for merging if things looks okay.

@arun-shopify I don't have permission to merge. By the way, it looks like there are merge conflicts that need resolving?

@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from 9f93fa0 to 50ae063 Compare October 11, 2022 13:55
@arun-shopify
setting TraceID and SpanID fields in Exemplar objects in spanmetricsp…
8a50034 
…rocessor and removing the use of FilteredAttributes to pass these values around.
@arun-shopify arun-shopify force-pushed the spanmetrics_exemplars_type_fields branch from 50ae063 to 8a50034 Compare October 11, 2022 17:12
@arun-shopify
Copy link
Contributor Author

@Aneurysm9 merge conflicts are resolved but that may short lived until something gets merged in the main branch.

@Aneurysm9 Aneurysm9 added the ready to merge Code review completed; ready to merge by maintainers label Oct 11, 2022
@codeboten codeboten merged commit 8f8a634 into open-telemetry:main Oct 13, 2022
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
@arun-shopify @shalper2
setting TraceID and SpanID fields in Exemplar objects in spanmetricsp… (
acd6c88 
open-telemetry#13401)

setting TraceID and SpanID fields in Exemplar objects in spanmetricsprocessor and removing the use of FilteredAttributes to pass these values around.
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bogdandrutu bogdandrutu bogdandrutu left review comments

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@albertteoh albertteoh albertteoh approved these changes

@djaglowski djaglowski Awaiting requested review from djaglowski djaglowski is a code owner automatically assigned from open-telemetry/collector-contrib-approvers

Assignees

@Aneurysm9 Aneurysm9

Labels
bug Something isn't working processor/spanmetrics Span Metrics processor ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@arun-shopify @bogdandrutu @albertteoh @Aneurysm9 @codeboten @dashpole