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

Bump opentelemetry to v0.15.0 #2634

Merged
merged 3 commits into from
Nov 21, 2020

Conversation

pkositsyn
Copy link
Contributor

@pkositsyn pkositsyn commented Nov 19, 2020

Which problem is this PR solving?

Update of dependency

Short description of the changes

The only real thing is that any pdata.Traces component is non-nullable now, so IsNil() and InitEmpty() became deprecated.

No changes applied to logic. The config for jaegerreceiver was updated in opentelemetry (initiated and implemented by me actually), but it is orthogonal to current jaeger code.

@pkositsyn pkositsyn requested a review from a team as a code owner November 19, 2020 23:26
Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #2634 (35ff45e) into master (e114201) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2634      +/-   ##
==========================================
- Coverage   95.08%   95.05%   -0.03%     
==========================================
  Files         213      213              
  Lines        9492     9492              
==========================================
- Hits         9025     9023       -2     
- Misses        390      391       +1     
- Partials       77       78       +1     
Impacted Files Coverage Δ
plugin/storage/badger/spanstore/reader.go 95.37% <0.00%> (-0.72%) ⬇️

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 e114201...35ff45e. Read the comment docs.

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
joe-elliott
joe-elliott previously approved these changes Nov 20, 2020
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

This looks good to me. I do have one question about these proto wrappers. Any insight you have would be appreciated.

@@ -248,7 +248,7 @@ func traceIDToString(high, low uint64) string {
}

func (c *Translator) process(resource pdata.Resource) *dbmodel.Process {
if resource.IsNil() || resource.Attributes().Len() == 0 {
if resource.Attributes().Len() == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

It seems that IsNil() was removed and the resource is guaranteed to never be nil?

I find these generate proto wrappers very confusing. Do you have any insight into why this was removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that went from my issue about panic in marshalling open-telemetry/opentelemetry-collector#1985. Then gradually all proto things became non-nullable. I believe, that using pointers was a mistake from the very beginning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, resource now cannot be nil

@@ -206,13 +206,15 @@ func createExporters(component ComponentType, storageTypes string, factories com

func enableAgentUDPEndpoints(jaeger *jaegerreceiver.Config) {
if jaeger.ThriftCompact == nil {
jaeger.ThriftCompact = &confignet.TCPAddr{
jaeger.ThriftCompact = &jaegerreceiver.ProtocolUDP{
Copy link
Member

Choose a reason for hiding this comment

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

Nice. This will allow us to support max udp packet size in the Jaeger otel collectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it is already configurable in otel receiver config

Copy link
Contributor Author

@pkositsyn pkositsyn Nov 20, 2020

Choose a reason for hiding this comment

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

Alongwith socket buffer size, number of workers and server queue size

Signed-off-by: Pavel Kositsyn <kositsyn.pa@phystech.edu>
@mergify mergify bot dismissed joe-elliott’s stale review November 20, 2020 01:45

Pull request has been modified.

@pkositsyn
Copy link
Contributor Author

pkositsyn commented Nov 20, 2020

This is a flaky test I suppose. Locally, tests pass periodically.

@joe-elliott joe-elliott self-requested a review November 20, 2020 21:27
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Looks good.

@yurishkuro yurishkuro merged commit 2a0d0be into jaegertracing:master Nov 21, 2020
@pkositsyn pkositsyn deleted the bump_opentelemetry_v15 branch November 21, 2020 05:46
quinniup pushed a commit to k8battleship/jaeger that referenced this pull request Nov 23, 2020
* 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>
quinniup added a commit to k8battleship/jaeger that referenced this pull request Nov 23, 2020
* 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>
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.

3 participants