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

fix(exporter-zipkin): round timestamp to nearest integer in zipkin annotations #4167

Conversation

FelipeEmerim
Copy link
Contributor

@FelipeEmerim FelipeEmerim commented Sep 27, 2023

Which problem is this PR solving?

Exporter-zipkin was not rounding the timestamp field on annotations which causes 400 errors when traces include annotations.

Fixes #4165

Short description of the changes

Round the timestamp field to the nearest integer on every annotation.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.

  • Unit tests for zipkin exporter
  • Used npm-link to apply this patch to a project and export traces to a local otel-collector using zipkin receiver

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@FelipeEmerim FelipeEmerim requested a review from a team as a code owner September 27, 2023 16:46
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 27, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: FelipeEmerim (bba7814)
  • ✅ login: pichlermarc / name: Marc Pichler (6a60367)

@FelipeEmerim FelipeEmerim force-pushed the fix/round-timestamp-in-zipkin-annotations branch from 411c40d to 70d6c4d Compare September 27, 2023 16:47
@FelipeEmerim FelipeEmerim force-pushed the fix/round-timestamp-in-zipkin-annotations branch from 70d6c4d to bba7814 Compare September 27, 2023 18:48
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #4167 (f1a9241) into main (513d067) will decrease coverage by 0.10%.
The diff coverage is n/a.

❗ Current head f1a9241 differs from pull request most recent head 6a60367. Consider uploading reports for the commit 6a60367 to get more accurate results

@@            Coverage Diff             @@
##             main    #4167      +/-   ##
==========================================
- Coverage   92.19%   92.10%   -0.10%     
==========================================
  Files         324      328       +4     
  Lines        9228     9235       +7     
  Branches     1935     1955      +20     
==========================================
- Hits         8508     8506       -2     
- Misses        720      729       +9     
Files Coverage Δ
...ges/opentelemetry-exporter-zipkin/src/transform.ts 90.90% <ø> (ø)

... and 11 files with indirect coverage changes

@pichlermarc pichlermarc merged commit f0ceabc into open-telemetry:main Sep 28, 2023
17 checks passed
@FelipeEmerim FelipeEmerim deleted the fix/round-timestamp-in-zipkin-annotations branch October 6, 2023 17:21
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.

Zipkin Exporter - Cannot export spans because timestamp in annotations is not an integer
2 participants