Implement new HTTP semantic convention opt-in for Falcon #2790
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emdneto
reviewed
Aug 9, 2024
| @@ -66,6 +70,7 @@ def setUp(self): | |||
| { | |||
| "OTEL_PYTHON_FALCON_EXCLUDED_URLS": "ping", | |||
| "OTEL_PYTHON_FALCON_TRACED_REQUEST_ATTRS": "query_string", | |||
| "OTEL_SEMCONV_STABILITY_OPT_IN": "http/dup", | |||
There was a problem hiding this comment.
Are we missing the tests for "http" and "default"?
| @@ -243,6 +251,11 @@ class _InstrumentedFalconAPI(getattr(falcon, _instrument_app)): | |||
| def __init__(self, *args, **kwargs): | |||
| otel_opts = kwargs.pop("_otel_opts", {}) | |||
|
|
|||
| _OpenTelemetrySemanticConventionStability._initialize() | |||
There was a problem hiding this comment.
I think we are missing some important points here:
Checklist
- Add semconv status as
migrationfor falcon in instrumentations README - Populate
{method}asHTTPwhen nonstandard method (sanitize_method) - set request HTTP method attribute as
_OTHERwhen nonstandard http method and for new semconv also sethttp.request.method_originalwith the original nonstandard method. - Set schema_url based on the opt-in mode
- Create histograms based on the opt-in mode (different metric names so no dup attributes)
- Set metrics attributes based on the opt-in mode
- Set span attributes based on the opt-in mode
- Tests for new_semconv, both_semconv and default
There was a problem hiding this comment.
Thank you for the great feedback, I'll get started on this.
lzchen
reviewed
Aug 12, 2024
| @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| - `opentelemetry-instrumentation-kafka-python` Instrument temporary fork, kafka-python-ng | |||
| inside kafka-python's instrumentation | |||
| ([#2537](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2537))) | |||
| - `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon | |||
There was a problem hiding this comment.
Suggested change
| - `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon | |
| - `opentelemetry-instrumentation-falcon` Implement new HTTP semantic convention opt-in for Falcon | |
| ([#2790](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2790)) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change updates the falcon instrumentor to adhere to the new HTTP semantic conventions, according to the migration plan outlined here:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Updated the tests to assert that the new span and duration attributes have been added.