Skip to content

[OTel C++] Add API to set channel scope filter#36189

Closed
yashykt wants to merge 6 commits intogrpc:masterfrom
yashykt:OTelChannelScope
Closed

[OTel C++] Add API to set channel scope filter#36189
yashykt wants to merge 6 commits intogrpc:masterfrom
yashykt:OTelChannelScope

Conversation

@yashykt
Copy link
Copy Markdown
Member

@yashykt yashykt commented Mar 28, 2024

Also addressing a TODO from previous PRs where authority is not being populated in the ChannelScope.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should probably refer to this type as OpenTelemetryPluginBuilder::ChannelScope, for consistency with the rest of this code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here: This should probably refer to this type as OpenTelemetryPluginBuilder::ChannelScope, for consistency with the rest of this code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

target, args.GetOwnedString(GRPC_ARG_DEFAULT_AUTHORITY).value_or(""));
experimental::StatsPluginChannelScope scope(
target, args.GetOwnedString(GRPC_ARG_DEFAULT_AUTHORITY)
.value_or(CoreConfiguration::Get()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've done some digging to remind myself of the flow here (details originally came from #14615 (comment)), and I've convinced myself that this approach makes sense. There's some more cleanup that will be needed here, but I'll take care of that in the v3 stack.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants