Bump opentelemetry-collector to v0.14.0 #2617

pkositsyn · 2020-11-06T19:19:19Z

Which problem is this PR solving?
Adds compatibility with new version of collector

Short description of the changes

Just refactoring mostly. There is only one open question: should retry queue be enabled by default?
Collector defaults are to enable it, but according to tests it should be disabled

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

cmd/opentelemetry/app/exporter/elasticsearchexporter/esmodeltranslator/modeltranslator_test.go

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

jpkrohling

LGTM

yurishkuro · 2020-11-06T20:41:10Z

cmd/opentelemetry/app/exporter/badgerexporter/factory.go

@@ -91,17 +91,17 @@ func (f Factory) CreateDefaultConfig() configmodels.Exporter {

 // CreateTraceExporter creates Jaeger Cassandra trace exporter.
 // This function implements OTEL component.ExporterFactory interface.
-func (f Factory) CreateTraceExporter(
+func (f Factory) CreateTracesExporter(


hehe, what happens when so many people contributing are not native speakers. "trace exporter" was grammatically correct name.

not a jab at you, @Vemmy124 , but on the upstream change

I confess it did sound strange to my ears, but I'm not a native speaker myself, so, thought it was a problem with my ears :-)

I believe that was bogdandrutu's idea open-telemetry/opentelemetry-collector#1974

And maybe it's really the problem of me being not native speaker, but I agree with him, because the component consumes the structure named pdata.Traces

It doesn't matter what exact data type is passed, a component that exports traces (plural) is "trace exporter" (noun used as adjective in singular form).

yurishkuro · 2020-11-06T20:47:34Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/esmodeltranslator/modeltranslator.go

-	if err != nil {
-		return "", err
-	}
+	spanIDInt := tracetranslator.BytesToUInt64SpanID(spanID.Bytes())
 	if spanIDInt == 0 {
 		return "", errZeroSpanID
 	}
 	return dbmodel.SpanID(fmt.Sprintf("%016x", spanIDInt)), nil


This probably would be more efficient:

import "encoding/hex" src := spanID.Bytes() dst := make([]byte, hex.EncodedLen(len(src))) hex.Encode(dst, src) return dbmodel.SpanID(dst)

According to benchmark run on my device, this change gave 70ns vs previous 170ns

yurishkuro · 2020-11-06T20:49:31Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/esmodeltranslator/modeltranslator.go

-	if err != nil {
-		return "", err
-	}
+	spanIDInt := tracetranslator.BytesToUInt64SpanID(spanID.Bytes())
 	if spanIDInt == 0 {
 		return "", errZeroSpanID


This feels misplaced to me. This kind of validation should've been done way earlier in the pipeline. The ES backend itself doesn't actually care if IDs are all zeroes, it would still work.

You're definitely right, but practically having this error handled here can prevent some time of unnecessary debugging in case of zero IDs and missing validation earlier. I believe this error arises systematically, so anyway there will be collisions by ID and this will only implicitly indicate zero ID error.

yurishkuro · 2020-11-06T20:51:18Z

cmd/opentelemetry/app/exporter/elasticsearchexporter/esmodeltranslator/modeltranslator_test.go

-		{
-			traceID: pdata.NewTraceID([]byte("invalid-%")),
-			err:     "TraceID does not have 16 bytes",
-		},


not sure why these tests are removed - give the current code there would still be some kind of error returned from converters.

But TraceID is now exactly [16]byte. Why would the error be there even if only several first bytes are set?

They are silently truncated according to this PR conversations open-telemetry/opentelemetry-collector#2001

yurishkuro

lgtm, but why are some tests removed?

pkositsyn · 2020-11-08T22:10:14Z

Seems like a problem with build. Could you take a look please, since you're acquainted with how tests are run?

yurishkuro · 2020-11-08T23:29:37Z

Lint Failures
cmd/opentelemetry/app/exporter/badgerexporter/factory.go:92:1: comment on exported method Factory.CreateTracesExporter should be of the form "CreateTracesExporter ..."
cmd/opentelemetry/app/exporter/cassandraexporter/factory.go:74:1: comment on exported method Factory.CreateTracesExporter should be of the form "CreateTracesExporter ..."
cmd/opentelemetry/app/exporter/elasticsearchexporter/factory.go:75:1: comment on exported method Factory.CreateTracesExporter should be of the form "CreateTracesExporter ..."
cmd/opentelemetry/app/exporter/grpcpluginexporter/factory.go:68:1: comment on exported method Factory.CreateTracesExporter should be of the form "CreateTracesExporter ..."
cmd/opentelemetry/app/exporter/jaegerexporter/jaeger_exporter.go:61:1: comment on exported method Factory.CreateTracesExporter should be of the form "CreateTracesExporter ..."
cmd/opentelemetry/app/exporter/kafkaexporter/kafka_exporter.go:100:1: comment on exported method Factory.CreateTracesExporter should be of the form "CreateTracesExporter ..."
cmd/opentelemetry/app/exporter/memoryexporter/factory.go:81:1: comment on exported method Factory.CreateTracesExporter should be of the form "CreateTracesExporter ..."
cmd/opentelemetry/app/processor/resourceprocessor/resource_processor.go:70:1: comment on exported method Factory.CreateTracesProcessor should be of the form "CreateTracesProcessor ..."
cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go:150:1: comment on exported method Factory.CreateTracesReceiver should be of the form "CreateTracesReceiver ..."
cmd/opentelemetry/app/receiver/kafkareceiver/kafka_receiver.go:107:1: comment on exported method Factory.CreateTracesReceiver should be of the form "CreateTracesReceiver ..."
cmd/opentelemetry/app/receiver/zipkinreceiver/zipkin_receiver.go:58:1: comment on exported method Factory.CreateTracesReceiver should be of the form "CreateTracesReceiver ..."
Makefile:198: recipe for target 'go-lint' failed

pkositsyn · 2020-11-09T07:53:20Z

I don't understand, how this piece of code passed lint before then. Anyways, let's add comments

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

Pull request has been modified.

pkositsyn · 2020-11-09T11:17:39Z

I wasn't right about the tests correction, fixed now. What is the reason of absence of golangci.yml in the main repo? Wouldn't it be useful for local checks?

codecov · 2020-11-09T11:20:45Z

Codecov Report

Merging #2617 (5b5e72e) into master (2a0d8cc) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2617      +/-   ##
==========================================
- Coverage   95.07%   95.04%   -0.04%     
==========================================
  Files         209      209              
  Lines        9364     9364              
==========================================
- Hits         8903     8900       -3     
- Misses        385      387       +2     
- Partials       76       77       +1

Impacted Files	Coverage Δ
plugin/storage/badger/factory.go	`95.78% <100.00%> (ø)`
plugin/storage/badger/spanstore/reader.go	`95.37% <100.00%> (ø)`
cmd/query/app/server.go	`88.52% <0.00%> (-1.64%)`	⬇️
plugin/storage/integration/integration.go	`77.34% <0.00%> (-0.56%)`	⬇️

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 740264b...5b5e72e. Read the comment docs.

pkositsyn · 2020-11-09T12:48:54Z

@yurishkuro @jpkrohling Can we merge now?

jpkrohling · 2020-11-09T13:03:59Z

There's one pending change requested by Yuri: #2617 (comment)

About the linting configuration: I think it would be a welcome change.

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

pkositsyn · 2020-11-09T14:46:53Z

@yurishkuro was right - new method for span ID conversion is a lot faster. Now I believe everything is resolved (well, idk if we reached consensus about spanID validation)

yurishkuro · 2020-11-09T17:40:19Z

Restarted build. I have not seen crossdock test failing too often recently, and here it failed several times, may be related to this change. But could be flaky too, let's see how the build comes back.

pkositsyn · 2020-11-09T19:07:34Z

FWIW in my build on real data some traces are lost somewhere between elasticexporter and elastic cluster. The worst thing is that there are no logs about loss in opentelemetry-collector component until v0.14.0. This might be closely related actually and I will investigate that once we merge this PR.

Defending my changes in commits, I have to notice, that tests passed with my previous commit, which (at least should) have equivalent semantics with the latest version.

yurishkuro · 2020-11-09T20:27:20Z

Pretty sure the crossdock_otel CI failure is related to this change - it consistently failed 3 times, while passing on master and other PRs.

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

Pull request has been modified.

pkositsyn · 2020-11-09T22:21:50Z

Found the problem. There was no test on the case when only 8 bytes of trace id is generated. Corrected tests to check that as well

yurishkuro · 2020-11-09T23:04:42Z

Thanks!

Go phystech.edu :-)

* bump opentelemetry-collector to v0.14.0 Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * initialize traceid and spanid explicitly Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * fix comments and empty parent span check Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * fasten convert traceID/spanID Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * fix convertTraceID + fix tests Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

* Bump opentelemetry-collector to v0.14.0 (jaegertracing#2617) * bump opentelemetry-collector to v0.14.0 Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * initialize traceid and spanid explicitly Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * fix comments and empty parent span check Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * fasten convert traceID/spanID Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * fix convertTraceID + fix tests Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * Update CodeQL to latest best practices (jaegertracing#2615) This will parallelize your analysis and speed things up a bunch. Signed-off-by: jhutchings1 <jhutchings1@users.noreply.github.com> Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Fix flaky TestReload (jaegertracing#2624) Signed-off-by: albertteoh <albert.teoh@logz.io> * Update x/text to v0.3.4 (jaegertracing#2625) Signed-off-by: Gary Brown <gary@brownuk.com> * Bump to latest UI for snapshot builds (jaegertracing#2626) Signed-off-by: Yuri Shkuro <github@ysh.us> * Implement anonymizer's main program (jaegertracing#2621) * Preparing release 1.21.0 (jaegertracing#2630) * updated changelog Signed-off-by: Joe Elliott <number101010@gmail.com> * Added ui changelog Signed-off-by: Joe Elliott <number101010@gmail.com> * Fixed UI changelog to point to 1.12.0 Signed-off-by: Joe Elliott <number101010@gmail.com> * Updated jaeger-ui to v1.12.0 Signed-off-by: Joe Elliott <number101010@gmail.com> * Resolving concerns Signed-off-by: Joe Elliott <number101010@gmail.com> * [anonymizer] Save trace in UI format (jaegertracing#2629) * Use fossa-contrib/fossa-action instead (jaegertracing#2571) * Use fossa-contrib/fossa-action instead Signed-off-by: Sora Morimoto <sora@morimoto.io> * Make step name clearer Signed-off-by: Sora Morimoto <sora@morimoto.io> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> * Update Makefile and Dockerfile for anonymizer (jaegertracing#2632) Signed-off-by: Ashmita Bohara <ashmita.bohara152@gmail.com> * Fix listen IP in unit test (jaegertracing#2636) Signed-off-by: zouyu <zouy.fnst@cn.fujitsu.com> * Bump opentelemetry to v0.15.0 (jaegertracing#2634) * Bump opentelemetry to v0.15.0 Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * add default value instead of nil value for jaegerreceiver config Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> * make lint Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu> Co-authored-by: Kositsyn Pavel <vemmy124@gmail.com> Co-authored-by: Justin Hutchings <jhutchings1@users.noreply.github.com> Co-authored-by: Juraci Paixão Kröhling <juraci@kroehling.de> Co-authored-by: Albert <26584478+albertteoh@users.noreply.github.com> Co-authored-by: Gary Brown <gary.pi4tech@gmail.com> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Co-authored-by: Ashmita <ashmita.bohara152@gmail.com> Co-authored-by: Joe Elliott <number101010@gmail.com> Co-authored-by: Sora Morimoto <sora@morimoto.io> Co-authored-by: ZouYu <zouy.fnst@cn.fujitsu.com> Co-authored-by: Kositsyn Pavel <kositsyn.pa@phystech.edu>

pkositsyn requested a review from a team as a code owner November 6, 2020 19:19

pkositsyn requested a review from yurishkuro November 6, 2020 19:19

bump opentelemetry-collector to v0.14.0

64633e1

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

pkositsyn force-pushed the bump_opentelemetry branch from 1273fc6 to 64633e1 Compare November 6, 2020 19:25

pkositsyn commented Nov 6, 2020

View reviewed changes

cmd/opentelemetry/app/exporter/elasticsearchexporter/esmodeltranslator/modeltranslator_test.go Outdated Show resolved Hide resolved

initialize traceid and spanid explicitly

fc8f5d3

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

jpkrohling previously approved these changes Nov 6, 2020

View reviewed changes

yurishkuro reviewed Nov 6, 2020

View reviewed changes

yurishkuro previously approved these changes Nov 6, 2020

View reviewed changes

fix comments and empty parent span check

f6a0e29

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

pkositsyn force-pushed the bump_opentelemetry branch from 6dc93bc to f6a0e29 Compare November 9, 2020 11:07

fasten convert traceID/spanID

a78e918

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

pkositsyn force-pushed the bump_opentelemetry branch from c87a78b to a78e918 Compare November 9, 2020 13:35

yurishkuro previously approved these changes Nov 9, 2020

View reviewed changes

fix convertTraceID + fix tests

5b5e72e

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>

yurishkuro approved these changes Nov 9, 2020

View reviewed changes

yurishkuro merged commit 80805d2 into jaegertracing:master Nov 9, 2020

pkositsyn deleted the bump_opentelemetry branch November 10, 2020 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump opentelemetry-collector to v0.14.0 #2617

Bump opentelemetry-collector to v0.14.0 #2617

pkositsyn commented Nov 6, 2020

jpkrohling left a comment

yurishkuro Nov 6, 2020

yurishkuro Nov 6, 2020

jpkrohling Nov 6, 2020

pkositsyn Nov 6, 2020

pkositsyn Nov 8, 2020 •

edited

Loading

yurishkuro Nov 9, 2020

yurishkuro Nov 6, 2020 •

edited

Loading

pkositsyn Nov 9, 2020

yurishkuro Nov 6, 2020

pkositsyn Nov 8, 2020

yurishkuro Nov 6, 2020

pkositsyn Nov 6, 2020

pkositsyn Nov 6, 2020

yurishkuro left a comment

pkositsyn commented Nov 8, 2020

yurishkuro commented Nov 8, 2020

pkositsyn commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

codecov bot commented Nov 9, 2020 •

edited

Loading

pkositsyn commented Nov 9, 2020

jpkrohling commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

yurishkuro commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

yurishkuro commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

yurishkuro commented Nov 9, 2020

Bump opentelemetry-collector to v0.14.0 #2617

Bump opentelemetry-collector to v0.14.0 #2617

Conversation

pkositsyn commented Nov 6, 2020

Short description of the changes

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkositsyn Nov 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

pkositsyn commented Nov 8, 2020

yurishkuro commented Nov 8, 2020

pkositsyn commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

codecov bot commented Nov 9, 2020 • edited Loading

Codecov Report

pkositsyn commented Nov 9, 2020

jpkrohling commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

yurishkuro commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

yurishkuro commented Nov 9, 2020

pkositsyn commented Nov 9, 2020

yurishkuro commented Nov 9, 2020

pkositsyn Nov 8, 2020 •

edited

Loading

yurishkuro Nov 6, 2020 •

edited

Loading

codecov bot commented Nov 9, 2020 •

edited

Loading