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 trace jaeger conversion to internal traces zero time bug #1957

Merged
merged 4 commits into from
Oct 14, 2020

Conversation

pkositsyn
Copy link
Contributor

Description:
Fixing a bug. When spans are converted from internal trace to jaeger trace the conversion is zero timestamp -> time.Time{} which is a reserved date. When converted back, this date is interpreted as a usual one, so ProtoBatchesToInternalTraces(InternalTracesToJaegerProto(traces)) != traces. So I added a check on this one

@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #1957 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1957   +/-   ##
=======================================
  Coverage   91.41%   91.41%           
=======================================
  Files         284      284           
  Lines       16788    16791    +3     
=======================================
+ Hits        15347    15350    +3     
  Misses       1009     1009           
  Partials      432      432           
Impacted Files Coverage Δ
consumer/pdata/timestamp.go 100.00% <100.00%> (ø)
translator/trace/jaeger/jaegerproto_to_traces.go 91.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 91.78% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4016b2b...1db3f3e. Read the comment docs.

@tigrannajaryan
Copy link
Member

Please add a test to verify the bug fix.

@tigrannajaryan tigrannajaryan self-assigned this Oct 14, 2020
@pkositsyn
Copy link
Contributor Author

Well, I think it is better to add function to timestamp package for that and test it there

@pkositsyn
Copy link
Contributor Author

@tigrannajaryan does it look good now?

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. Thanks.

Collector automation moved this from In progress to Reviewer approved Oct 14, 2020
@tigrannajaryan tigrannajaryan merged commit b0a3e43 into open-telemetry:master Oct 14, 2020
Collector automation moved this from Reviewer approved to Done Oct 14, 2020
@pkositsyn pkositsyn deleted the bug_jaeger_zero_time branch October 15, 2020 16:25
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants