Add declarative config support for sensitive_query_parameters

[trask]

Depends on open-telemetry/opentelemetry-configuration#531

[jack-berg]

Do you like these helper functions?

[trask]

good question, I guess not really 😄

[jack-berg]

My initial thought is that they would be helpful for instrumentation conform to these general config options. All the instrumentation in opentelemetry-java-instrumentation can leverage shared utils in that repo to do this type of thing, so its really only native instrumentation that would stand to benefit.

[trask]

I kind of like the explicitness of seeing the yaml path in the code.

But can see them being useful.

What's your thought about these methods handling default values (and never returning null)?

[jack-berg]

What's your thought about these methods handling default values (and never returning null)?

I support. Their value proposition decreases if the caller still has to be aware of the default semantics.

[trask]

55cbbb6

[jack-berg]

I guess I didn't realize that the defaults would be defined per applicable semantic convention: https://github.com/open-telemetry/opentelemetry-configuration/pull/531/changes#diff-d77a4fdd93a40cc4f4e39de58670cb50a8f0cfed9d012f4960d773a9f50642d6R165

Does this suggest that the sensitive_query_parameters property is better suited under .instrumentation/development.general.http instead of .instrumentation/development.general.sanitization.url?

Or the helper function needs to be renamed to reflect that this is sensitive query parameters for HTTP instrumentation.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.32%. Comparing base (6b98c74) to head (55cbbb6).

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8087   +/-   ##
=========================================
  Coverage     90.32%   90.32%           
+ Complexity     7607     7604    -3     
=========================================
  Files           839      839           
  Lines         22888    22913   +25     
  Branches       2283     2283           
=========================================
+ Hits          20673    20697   +24     
  Misses         1506     1506           
- Partials        709      710    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jkwatson]

Having these instrumentation concerns leaking into the core repo is weirding me out a bit. I wonder if this is the right place for this stuff to live, given that the semconv implementations live in a different place from here.

[jack-berg]

Having these instrumentation concerns leaking into the core repo is weirding me out a bit. I wonder if this is the right place for this stuff to live, given that the semconv implementations live in a different place from here.

Given that these are just utility methods, we could move them to an existing (or new) module in instrumentation and achieve the same affect? Might be helpful for instrumentation maintainers to "vertically integrate", owning the utility methods that native instrumentation as well as opentelemetry-java-instrumentation depends on.

[trask]

Instrumentation API users don't need these utils since we will bake the behavior into the semconv specific AttributesExtractors.

[jack-berg]

Idea: since instrumentation config schema is sourced from semantic conventions, should we think of these types of helper functions as another type of generated code to be packaged / published from semantic-conventions-java?

From a scope perspective, could grow to think of semantic-conventions-java as generated constants / code for all instrumentation needs which arise from semconv, including attributes, metric metadata, event metadata, config, and anything else relevant.