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

Add OC to OTLP translation functions #546

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Add OC to OTLP translation functions #546

merged 1 commit into from
Feb 18, 2020

Conversation

tigrannajaryan
Copy link
Member

This implements OC to OTLP protobuf message translation.
The code is currently reachable only from the tests.

We probably need a bit more coverage for this code but I am
submitting this change as is for the sake of keeping the
size of the PR managable for reviewing.

Contributes to #478

@codecov-io
Copy link

codecov-io commented Feb 13, 2020 •
edited
Loading

Codecov Report

Merging #546 into master will increase coverage by 0.14%.
The diff coverage is 80.32%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   76.19%   76.33%   +0.14%     
==========================================
  Files         129      130       +1     
  Lines        7993     8228     +235     
==========================================
+ Hits         6090     6281     +191     
- Misses       1602     1643      +41     
- Partials      301      304       +3
Impacted Files Coverage Δ
consumer/consumerdata/consumerdata.go 0% <0%> (ø) ⬆️
internal/internal.go 45.45% <0%> (-25.98%) ⬇️
translator/trace/spandata/protospan_to_spandata.go 60.11% <75%> (+0.23%) ⬆️
translator/trace/oc_to_otlp.go 82.83% <82.83%> (ø)

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 2edc543...4f87c73. Read the comment docs.

owais
owais reviewed Feb 13, 2020
View reviewed changes
consumer/consumerdata/consumerdata.go
@@ -41,13 +41,17 @@ type TraceData struct {
// OTLPTraceData is a struct that groups proto spans with a resource. This is the
// newer version of TraceData, using OTLP-based representation.
type OTLPTraceData struct {
ResourceSpanList []*otlptrace.ResourceSpans
resourceSpanList []*otlptrace.ResourceSpans
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we making this private temporarily to prevent usage before this is ready?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's one of the reasons. The other is that I may keep it private and require certain modifications to go through functions. It will become clearer in future PRs. Let's keep it private for now.

internal/internal.go
return
}
return time.Unix(ts.Seconds, int64(ts.Nanos))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is quite useful in many places. I think we should have a package to deal with timestamps in general as I've seen this and very similar function scattered across packages in this and contrib repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this from Jaeger translation code. This can be now reused everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Later we should move it out of internal so contrib can also use it, a translation package seems a good candidate.

translator/trace/oc_to_otlp.go Show resolved Hide resolved
translator/trace/oc_to_otlp.go Outdated Show resolved Hide resolved
translator/trace/oc_to_otlp.go Outdated
DroppedEventsCount: droppedEventCount,
Links: links,
DroppedLinksCount: droppedLinkCount,
Status: nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentionally nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, thanks for catching! Completely missed the Status translation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, please have a look.

@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/translators branch 2 times, most recently from 7b1c6ed to 257516a Compare February 13, 2020 16:36
pjanotti
pjanotti reviewed Feb 13, 2020
View reviewed changes
translator/trace/spandata/protospan_to_spandata.go
@@ -47,8 +47,8 @@ func ProtoSpanToOCSpanData(span *tracepb.Span) (*trace.SpanData, error) {
sd := &trace.SpanData{
SpanContext: sc,
ParentSpanID: parentSpanID,
StartTime: timestampToTime(span.StartTime),
EndTime: timestampToTime(span.EndTime),
StartTime: internal.TimestampToTime(span.StartTime),
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed this earlier but internal is not a good name for a package.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not a new package. We can rename it to something better in an future PR.

pjanotti
pjanotti approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

owais
owais approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

LGTM

pjanotti
pjanotti reviewed Feb 13, 2020
View reviewed changes
translator/trace/oc_to_otlp.go Outdated Show resolved Hide resolved
translator/trace/oc_to_otlp.go Outdated Show resolved Hide resolved
translator/trace/oc_to_otlp.go Show resolved Hide resolved
translator/trace/oc_to_otlp.go Outdated Show resolved Hide resolved
translator/trace/oc_to_otlp.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member Author

@pjanotti please have another look.

pjanotti
pjanotti approved these changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

Add OC to OTLP translation functions
4f87c73 
This implements OC to OTLP protobuf message translation.
The code is currently reachable only from the tests.

We probably need a bit more coverage for this code but I am
submitting this change as is for the sake of keeping the
size of the PR managable for reviewing.

Contributes to open-telemetry#478
@tigrannajaryan tigrannajaryan merged commit dae8035 into open-telemetry:master Feb 18, 2020
@tigrannajaryan tigrannajaryan deleted the feature/tigran/translators branch February 18, 2020 15:17
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
@evantorrie @rghetia
Use StateLocker in MinMaxSumCount (open-telemetry#546)
8680783 
* Add MinMaxSumCount stress test

* Reimplement MinMaxSumCount using StateLocker

* Address PR comments

* Round open-telemetry#2 of PR comments

Co-authored-by: Rahul Patel <rahulpa@google.com>
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
@billg-splunk
Update components.md (open-telemetry#546)
c5e331b 
Believe this is called metricstransform (not metrictransform)
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
@ThomsonTan
Remove incorrection description on code.coverage in CI doc (open-tele…
a1dc8bd 
…metry#546)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owais owais owais approved these changes

@pjanotti pjanotti pjanotti approved these changes

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

@flands flands Awaiting requested review from flands

@rghetia rghetia Awaiting requested review from rghetia

@songy23 songy23 Awaiting requested review from songy23

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tigrannajaryan @codecov-io @owais @pjanotti