search

open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
817 stars 392 forks source link

Implement Baggage ( prev: CorrelationContext ) #79

Closed reyang closed 3 years ago

reyang commented 4 years ago

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/correlationcontext/api.md

nikhil1511 commented 3 years ago

Hi, I would like to work on this task. My understanding is that baggage will have multiple key, value pairs. Do we need new type something like shared_ptr of unordered_map<string, string> in the variant of contextval for the same ? Clarification on this will be helpful. Thanks in advance.

lalitb commented 3 years ago

@nikhil1511 - Thanks for showing interest on this task. Baggage extracts and inserts information in Context object ( https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/context/context.h). No new type should be needed for ContextValue. Suggest you to go through baggage specs, reference implementation in other languages, and come up with the PR for C++ if the task interests you. We can further discuss on that.

nikhil1511 commented 3 years ago

Thanks for the reply. But, baggage key "baggage" has multiple key value pairs as its value, right (example "key1=val1,key2=val2") ? So, how do we plan to support this in context ? Ideally "baggage" will be key in the context and value should be data structure representing "key1=val1,key2=val2". Am I missing anything here ? I have taken reference from python apis.

lalitb commented 3 years ago

If required should be fine to create a new type in variant. Just ensure not to use any STL types (vector, maps, unordered_map ) within api intrfaces and as data member within classes in api to ensure ABI stability: https://github.com/open-telemetry/opentelemetry-cpp/blob/484700a0da2547852653271d2bdc10b50830bb86/docs/abi-policy.md

nikhil1511 commented 3 years ago

Hello Lalit, I have implemented api, context interaction and propagation for baggage with unit tests. Currently, it is in my forked repo( https://github.com/nikhil1511/opentelemetry-cpp/pull/1 ). You have raised PR for baggage api already, how would you suggest me to go forward. Your inputs will be very helpful. Apologies for delay from my side after last conversation. PS. I had taken some references from trace_state implementation. The duplicated code has been already moved to common place in your PR, I can adjust my prs accordingly.

lalitb commented 3 years ago

@nikhil1511 - My PR is still draft. I see your changes in right direction . Can you go through my PR once and incorporate required changes from there. Basically what we want is:

We need baggage api merged for our beta release which is planned for March end. See if we can come up with something soon.

lalitb commented 3 years ago

Also if you like to join our community meeting to discuss further, feel free to join coming week. You can find meeting details in repo root.

nikhil1511 commented 3 years ago

@nikhil1511 - My PR is still draft. I see your changes in right direction . Can you go through my PR once and incorporate required changes from there. Basically what we want is:

  • TraceState and Baggage should reuse most of the code, instead of duplication at both the places. Also see if we can reuse header parsing part.
  • Only allocate needed memory. Current implementation allocated max possible entries which can be improved.

We need baggage api merged for our beta release which is planned for March end. See if we can come up with something soon.

@lalitb working on incorporating changes from your pr. I will try to put three separate PRs instead of my current PR where all following features are added

Do let me know if you think otherwise.

nikhil1511 commented 3 years ago

@lalitb should I raise a new PR from main branch for trace_state refactoring ? I have kept many changes from your draft PR and extracted out some parsing logic of header string to common place. Also, planning to remove baggage part from the PR and add it in subsequent prs [Unit tests are not complete yet. So, I will raise after tests are complete.] Or should I put patch on your draft PR ?

lalitb commented 3 years ago

@lalitb should I raise a new PR from main branch for trace_state refactoring ? I have kept many changes from your draft PR and extracted out some parsing logic of header string to common place. Also, planning to remove baggage part from the PR and add it in subsequent prs [Unit tests are not complete yet. So, I will raise after tests are complete.] Or should I put patch on your draft PR ?

Better to raise a new PR for this.

nikhil1511 commented 3 years ago

@lalitb I have created a PR to be submitted to main branch. I am just waiting for couple of legal approvals (should be done quickly, I hope). Just wanted to give update. Link : https://github.com/nikhil1511/opentelemetry-cpp/pull/2