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

Assign correct value to ext_dt_spanId in Tld Geneva exporter #1184

Merged
merged 3 commits into from
May 12, 2023
Merged

Assign correct value to ext_dt_spanId in Tld Geneva exporter #1184

merged 3 commits into from
May 12, 2023

Conversation

manio143
Copy link
Contributor

@manio143 manio143 commented May 11, 2023 •
edited
Loading

Changes

I was testing the Geneva TldExporter locally (it produces very nice info into the EventSource) and I noticed the spanId value was too long and the same as traceId, so I looked in the code and noticed it was assigning the wrong property to the field.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@manio143 manio143 requested a review from a team May 11, 2023 10:36
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 11, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: manio143 / name: Marian Dziubiak (bf49696)

@manio143 manio143 force-pushed the geneva-tld-spanid-fix branch from 25712cb to bf49696 Compare May 11, 2023 10:39
@Kielek Kielek added the comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva label May 11, 2023
@manio143
Copy link
Contributor Author

@manio143, please sign CLA agreement by EasyCLA

Done 👍

Havret
Havret approved these changes May 11, 2023
View reviewed changes
Copy link
Contributor

@Havret Havret left a comment

Choose a reason for hiding this comment

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

lgtm

reyang
reyang approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with an entry in CHANGELOG.md.

@manio143
Copy link
Contributor Author

LGTM with an entry in CHANGELOG.md.

Do you want me to make another change as part of this PR?

cijothomas
cijothomas approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

good catch!

@cijothomas
Copy link
Member

LGTM with an entry in CHANGELOG.md.

Do you want me to make another change as part of this PR?

Yes please, to add changelog entry!

@cijothomas cijothomas merged commit d3f70ef into open-telemetry:main May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cijothomas cijothomas cijothomas approved these changes

@Havret Havret Havret approved these changes

@reyang reyang reyang approved these changes

@utpilla utpilla utpilla approved these changes

@CodeBlanch CodeBlanch Awaiting requested review from CodeBlanch

@Yun-Ting Yun-Ting Awaiting requested review from Yun-Ting

Assignees

@cijothomas cijothomas

@Yun-Ting Yun-Ting

@reyang reyang

@CodeBlanch CodeBlanch

@utpilla utpilla

Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@manio143 @cijothomas @Havret @reyang @utpilla @Kielek @Yun-Ting @CodeBlanch