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

feat(zipkin) include http path to span name #8150

Merged
merged 6 commits into from
Apr 1, 2022

Conversation

yankun-li-kong
Copy link
Contributor

@yankun-li-kong yankun-li-kong commented Dec 7, 2021

Add a new parameter span_include_path to decide whether to include http path to span name.
Add test case.

Summary

This PR is based on Kong/kong-plugin-zipkin#124, Thanks @XinweiLiu28
Able to include HTTP path to span name for zipkin plugin

Full changelog

Add a new parameter span_include_path to decide whether to include HTTP path to span name.
Add test case.

Issues resolved

Fix FTI-2842

Add a new parameter span_include_path to decide whether to include http path to span name.
Add test case.
kong/plugins/zipkin/handler.lua Outdated Show resolved Hide resolved
kong/plugins/zipkin/schema.lua Outdated Show resolved Hide resolved
@mayocream
Copy link
Contributor

I think it is good to have the endpoint in Span name, see OpenTelemetry spec opentelemetry-specification/api.md at main · open-telemetry/opentelemetry-specification

For example, here are potential span names for an endpoint that gets a hypothetical account information:

Span Name Guidance
get Too general
get_account/42 Too specific
get_account Good, and account_id=42 would make a nice Span attribute
get_account/{accountId} Also good (using the "HTTP route")

@mayocream mayocream self-assigned this Feb 9, 2022
kong/plugins/zipkin/schema.lua Outdated Show resolved Hide resolved
@kikito
Copy link
Member

kikito commented Mar 25, 2022

This looks ok to me. Could you please include a line on the changelog before merging it?

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.

4 participants