Skip to content

Feature/enable resource attributes #365

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Feature/enable resource attributes #365

wants to merge 2 commits into from

Conversation

royteeuwen
Copy link

No description provided.

@royteeuwen royteeuwen requested a review from a team January 23, 2024 21:17
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jan 23, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@royteeuwen royteeuwen force-pushed the feature/enable-resource-attributes branch from 5d1842c to 0071c1d Compare January 23, 2024 21:19
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 18, 2024
View reviewed changes
instrumentation/otel-webserver-module/src/nginx/ngx_http_opentelemetry_module.c
conf->nginxModuleEnabled,
(conf->nginxModuleOtelExporterEndpoint).data,
(conf->nginxModuleOtelExporterOtlpHeaders).data,
(conf->nginxModuleOtelResourceAttributes).data,

Check failure

Code scanning / CodeQL

Wrong type of arguments to formatting function

This argument should be of type 'long' but is of type 'unsigned char *'.
Copy link

Choose a reason for hiding this comment

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

I assume a few lines above the format string should be changed from

opentelemetry-cpp-contrib/instrumentation/otel-webserver-module/src/nginx/ngx_http_opentelemetry_module.c

Lines 1487 to 1488 in 0071c1d

"(OtelExporterOtlpHeader=\"%s\")"
"(OtelSslEnabled=\"%ld\")"
to add a flag for the new OtelResourceAttributes parameter:

                                                      "(OtelExporterOtlpHeader=\"%s\")"
                                                      "(OtelResourceAttributes=\"%s\")"
                                                      "(OtelSslEnabled=\"%ld\")"

Copy link
Author

Choose a reason for hiding this comment

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

@christophe-kamphaus-jemmic and can you merge this once I make the changes? This has been stale 6 months and I don't want to keep doing efforts while no one replies...

Copy link

Choose a reason for hiding this comment

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

I'm not a maintainer. I just saw this when I looked into #364.

You might want to join the Opentelemetry Slack channel #otel-cpp and/or the zoom meeting of the C/C++ SIG (every Wed 18:00 CEST) https://github.com/open-telemetry/community?tab=readme-ov-file#calendar and ask there.

@lalitb
Copy link
Member

lalitb commented Feb 29, 2024

@DebajitDas @kpratyus Can you please help here.

@marcalff marcalff added the Webserver This represents the otel-webserver-module in the instrumentation directory label Apr 13, 2024
@marcalff marcalff added the pr:fix-merge-conflicts Please fix merge conflicts for this pr label Apr 24, 2024
@krisfremen
Copy link

Hey there, any blockers on this getting reviewed and merged in?

I'm in a similar situation to what @royteeuwen encountered and would love to get this out in a version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpratyus kpratyus Awaiting requested review from kpratyus kpratyus is a code owner

@DebajitDas DebajitDas Awaiting requested review from DebajitDas DebajitDas is a code owner

1 more reviewer

@christophe-kamphaus-jemmic christophe-kamphaus-jemmic christophe-kamphaus-jemmic left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
pr:fix-merge-conflicts Please fix merge conflicts for this pr Webserver This represents the otel-webserver-module in the instrumentation directory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@royteeuwen @lalitb @krisfremen @christophe-kamphaus-jemmic @marcalff