Skip to content

[CAMEL-15196] adds support for opentracing tags#3919

Closed
jam01 wants to merge 6 commits into
apache:masterfrom
jam01:master
Closed

[CAMEL-15196] adds support for opentracing tags#3919
jam01 wants to merge 6 commits into
apache:masterfrom
jam01:master

Conversation

@jam01
Copy link
Copy Markdown
Contributor

@jam01 jam01 commented Jun 16, 2020

Opentracing allows to set tags to a given Span, this exposes that functionality via a processor.
See: https://issues.apache.org/jira/browse/CAMEL-15196
https://opentracing.io/docs/overview/tags-logs-baggage/#tags

I'm still implementing some tests, but wanted to check with you guys first in case I'm headed down the wrong path.

Copy link
Copy Markdown
Contributor

@oscerd oscerd left a comment

Choose a reason for hiding this comment

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

Just a checkstyle issue.

@jam01
Copy link
Copy Markdown
Contributor Author

jam01 commented Jun 16, 2020

Copy that, thank you both. As a follow up question, at what point does it make sense to make this a fully fledged component? There are also methods for setting and retrieving "baggage" from Spans. I'm wondering if multiple processors or a component would be appropriate.

@jam01 jam01 marked this pull request as ready for review July 9, 2020 04:17
@jam01 jam01 requested a review from oscerd July 9, 2020 04:21
@jam01
Copy link
Copy Markdown
Contributor Author

jam01 commented Jul 9, 2020

This PR should be good to go :) though I wrote a little bit more about its approach and an alternative one over at https://issues.apache.org/jira/browse/CAMEL-15196 if you have a minute to read it over

@oscerd
Copy link
Copy Markdown
Contributor

oscerd commented Jul 13, 2020

This PR should be good to go :) though I wrote a little bit more about its approach and an alternative one over at https://issues.apache.org/jira/browse/CAMEL-15196 if you have a minute to read it over

For my personal taste I'd prefer the second approach, but in terms of readability and correctness of implementation probably the processor approach seems better.

@jam01
Copy link
Copy Markdown
Contributor Author

jam01 commented Jul 14, 2020

Thanks @oscerd! If you have any directions about correctness of implementation I can definitely refactor it. Also, maybe we support both approaches?

@oscerd
Copy link
Copy Markdown
Contributor

oscerd commented Jul 14, 2020

I think we need to go with just one approach to avoid confusion, I guess this one is ok :-)

@jam01
Copy link
Copy Markdown
Contributor Author

jam01 commented Jul 14, 2020

Fair enough :)

@oscerd
Copy link
Copy Markdown
Contributor

oscerd commented Jul 16, 2020

I'll merge this today

@oscerd
Copy link
Copy Markdown
Contributor

oscerd commented Jul 16, 2020

Merged on master

@oscerd oscerd closed this Jul 16, 2020
@oscerd
Copy link
Copy Markdown
Contributor

oscerd commented Jul 16, 2020

Thanks

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