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

Adding functionality to add B3 propagator to the OtelWebserver NginxModule #403

Merged
merged 7 commits into from
Mar 28, 2024

Conversation

aryanishan1001
Copy link
Contributor

@aryanishan1001 aryanishan1001 commented Mar 26, 2024

Feature to enable the Nginx instrumentation to read and transport correlation readers in the B3 format.
#363

@aryanishan1001 aryanishan1001 requested a review from a team March 26, 2024 12:17
.gitignore Outdated
@@ -43,3 +43,5 @@
/out.*
# Indicator that the tools were deployed
.buildtools

.vscode
Copy link
Member

Choose a reason for hiding this comment

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

what does .vscode does here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not important, will remove.

@@ -108,6 +111,7 @@ inline std::ostream& operator<< (std::ostream &os, const otel::core::TenantConfi
<< "\n OtelExporterType " << config.getOtelExporterType()
<< "\n OtelExporterEndpoint " << config.getOtelExporterEndpoint()
<< "\n OtelProcessorType " << config.getOtelProcessorType()
<< "\n OtelPropagatorType " << config.getOtelPropagatorType()
Copy link
Member

Choose a reason for hiding this comment

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

nit: check the indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@@ -29,6 +29,7 @@ ServerSpan::ServerSpan(const std::string& name,
mLogger(logger)
{
// Extract W3C Trace Context. And store the token in local variable.

Copy link
Member

Choose a reason for hiding this comment

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

nit: remove extra line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

using MapHttpTraceCtx = opentelemetry::trace::propagation::HttpTraceContext;
mPropagators.push_back(
std::unique_ptr<MapHttpTraceCtx>(new MapHttpTraceCtx()));
if(config->getOtelPropagatorType() == "b3"){
Copy link
Member

Choose a reason for hiding this comment

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

Can we not have "b3" in a common place as const char* and use it here? Similarly we can have for w3c

Copy link
Member

Choose a reason for hiding this comment

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

Also, since this variable is set by user as string, there can be typo in such case. How are we handling the case when the variable is set as "B3" ? Where are we defaulting to when user set to some random characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not handling for "B3", it needs to "b3". When the entered string is not "b3", the if else statement will go for W3C headers.

@@ -44,7 +44,18 @@ void populatePayload(request_payload* req_payload, void* load)
payload->set_client_ip(req_payload->client_ip);

for(int i=0; i<req_payload->propagation_count; i++){
payload->set_http_headers(req_payload->propagation_headers[i].name, req_payload->propagation_headers[i].value);
if(strcmp(req_payload->propagation_headers[i].name , "x-b3-traceid") == 0){
Copy link
Member

Choose a reason for hiding this comment

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

why are "x-b3-traceid" used directly instead of some variable?

Copy link
Member

Choose a reason for hiding this comment

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

Also should not this Set be called under check of if type is b3 or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this stage of code, the program is just picking up all the propagation headers it has recieved.
At a later stage it decides if b3 is needed or w3c is needed.

std::unique_ptr<opentelemetry::trace::propagation::HttpTraceContext>(
new opentelemetry::trace::propagation::HttpTraceContext()));
}

Copy link
Member

Choose a reason for hiding this comment

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

Is that a fair understanding that at a time only one propagator can exist. Co-existence of both propagators is not supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could implement it in a co-existing way as well, but i think its better to only have one propagator at a time so that we have expected behaviour all the times.

using MapHttpTraceCtx = opentelemetry::trace::propagation::HttpTraceContext;
mPropagators.push_back(
std::unique_ptr<MapHttpTraceCtx>(new MapHttpTraceCtx()));
if(config->getOtelPropagatorType() == "b3"){
Copy link
Member

Choose a reason for hiding this comment

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

Also, since this variable is set by user as string, there can be typo in such case. How are we handling the case when the variable is set as "B3" ? Where are we defaulting to when user set to some random characters?

@@ -44,7 +44,18 @@ void populatePayload(request_payload* req_payload, void* load)
payload->set_client_ip(req_payload->client_ip);

for(int i=0; i<req_payload->propagation_count; i++){
payload->set_http_headers(req_payload->propagation_headers[i].name, req_payload->propagation_headers[i].value);
if(strcmp(req_payload->propagation_headers[i].name , "x-b3-traceid") == 0){
Copy link
Member

Choose a reason for hiding this comment

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

Also should not this Set be called under check of if type is b3 or not?

@DebajitDas DebajitDas merged commit 2f53439 into open-telemetry:main Mar 28, 2024
5 checks passed
@marcalff marcalff added the Webserver This represents the otel-webserver-module in the instrumentation directory label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Webserver This represents the otel-webserver-module in the instrumentation directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants