v3_preview applied for lowercase normalization for <name> in servlet.…#17822
v3_preview applied for lowercase normalization for <name> in servlet.…#17822trask merged 8 commits intoopen-telemetry:mainfrom
Conversation
…request.parameter.<name>
|
hi @IswaryaIyapalam! can you investigate the CI failures? thanks |
| @@ -1,3 +1,5 @@ | |||
| plugins { | |||
| id("otel.library-instrumentation") | |||
| id("otel.javaagent-instrumentation") | |||
There was a problem hiding this comment.
you shouldn't add this. Library instrumentations are standalone instrumentations that you include in you application as jar. Agent instrumetnations are bundled in side the javaagent. This module is a library instrumentation.
| // normalize parameter name similarly as is done with header names when header values are | ||
| // captured as span attributes | ||
| parameterName = parameterName.toLowerCase(Locale.ROOT); | ||
| if(!AgentCommonConfig.get().isV3Preview()) |
There was a problem hiding this comment.
This is a library instrumentation. AgentCommonConfig is a javaagent only class, it can't be used here.
There was a problem hiding this comment.
Hi @laurit, i referred the code. I assumed otel instrumentation api module can be used to get the common config and v3 preview flag. Please check the latest changes and let me know
| // normalize parameter name similarly as is done with header names when header values are | ||
| // captured as span attributes | ||
| parameterName = parameterName.toLowerCase(Locale.ROOT); | ||
| CommonConfig commonConfig = new CommonConfig(GlobalOpenTelemetry.get()); |
There was a problem hiding this comment.
If you add this here then it will be run when each key is created, perhaps it would be better to do this once in the constructor? It would be better to use GlobalOpenTelemetry.getOrNoop() since GlobalOpenTelemetry.get() prevents subsequent calls to GlobalOpenTelemetry.set(). Using the GlobalOpenTelemetry isn't guaranteed to work correctly for library instrumenations since they might not set it. To correctly handle library instrumenations you'd need to pass an OpenTelemetry instance to the constructor of ServletRequestParametersExtractor, but imo it isn't really important. The main idea behind this flag is to ease transitioning to the new defaults, so when we are finally ready to move to 3.0 we could search for the usages of this flag and flip the default instead of doing all these changes right before the release or maintaining a separate branch.
There was a problem hiding this comment.
There is a new method that you can use
There was a problem hiding this comment.
Thanks for highlighting the mistakes. Corrected the code as per your suggestion.
Co-authored-by: Trask Stalnaker <[email protected]>
|
Thank you for your contribution @IswaryaIyapalam! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
3 participants
fix given for this 15848 issue
First time contributing. Would be happy to get feedback if my understanding and fix is correct.