Skip to content

Upgrade io_opencensus_cpp.#8340

Merged
alyssawilk merged 2 commits intoenvoyproxy:masterfrom
g-easy:occ4
Oct 2, 2019
Merged

Upgrade io_opencensus_cpp.#8340
alyssawilk merged 2 commits intoenvoyproxy:masterfrom
g-easy:occ4

Conversation

@g-easy
Copy link
Copy Markdown
Contributor

@g-easy g-easy commented Sep 24, 2019

  • Fixes ocagent exporter bug where trace and span IDs were encoded
    incorrectly.

  • Adds support for service_name.

Signed-off-by: Emil Mikulic g-easy@users.noreply.github.com

* Fixes ocagent exporter bug where trace and span IDs were encoded
  incorrectly.

* Adds support for service_name.

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>
@g-easy
Copy link
Copy Markdown
Contributor Author

g-easy commented Sep 24, 2019

CI says ./tools/proto_format.sh mismatch, either run ./ci/do_ci.sh fix_format or ./tools/proto_format.sh fix to reformat. but I didn't touch protos.

@alyssawilk alyssawilk self-assigned this Sep 24, 2019
@alyssawilk
Copy link
Copy Markdown
Contributor

Yeah, looks like fix_format is broken on master - I'll see if I can sort that out ASAP
https://circleci.com/gh/envoyproxy/envoy/tree/master

Meanwhile if this is a bugfix, can you please add a unit test so we make sure we're regression-proof?
thanks!

@alyssawilk
Copy link
Copy Markdown
Contributor

ok #8345 will fix format issues, at which point you can do a master merge and it should resolve the format issues. Sorry for the noise!

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>
@g-easy
Copy link
Copy Markdown
Contributor Author

g-easy commented Sep 26, 2019

Done merge master. Thanks for the format fix! The bug was in occ code, not envoy code.

@alyssawilk
Copy link
Copy Markdown
Contributor

Sure, but if we're now passing clusterName correctly into opencensus, can't we check somewhere in tracer_test that it's now there?

@g-easy
Copy link
Copy Markdown
Contributor Author

g-easy commented Oct 2, 2019

The same question came up in #7354. The clusterName doesn't end up in e.g. the generated SpanData. The only thing we could meaningfully test in the Envoy driver code is the one line that puts it in the options struct. I think we should merge this as-is.

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

I think we should revisit code if we can't meaningfully test it
I'll buy that's out of scope of this PR but I think it's worth thinking about for the next one.

@alyssawilk alyssawilk merged commit c776fc3 into envoyproxy:master Oct 2, 2019
@g-easy g-easy deleted the occ4 branch October 3, 2019 02:04
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
Fixes ocagent exporter bug where trace and span IDs were encoded incorrectly.
Adds support for service_name.

Signed-off-by: Emil Mikulic <g-easy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants