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

Standardize config property names #1414

Closed
trask opened this issue Oct 18, 2020 · 12 comments
Closed

Standardize config property names #1414

trask opened this issue Oct 18, 2020 · 12 comments

Comments

@trask
Copy link
Member

trask commented Oct 18, 2020

I'd like to propose that we standardize config property names:

otel.instrumentation.<instrumentationId>.*

where <instrumentationId> is the instrumentation module name minus version suffix

This will match otel.instrumentation.<instrumentationId>.enabled which is used to suppress the instrumentation (#1412).

@mateuszrzeszutek
Copy link
Member

I've got a few questions in regards to this:

  • What about java.concurrent instrumentation? It uses otel.trace.executors and otel.trace.executors.all properties. It seems like it's a core/required instrumentation though so we may want to leave those names unchanged.
  • There are also otel.trace.methods, otel.trace.annotations and otel.trace.annotated.methods.exclude that are used in external annotations instrumentation, but I suppose that we don't really want to change them.

@trask
Copy link
Member Author

trask commented Nov 13, 2020

What about java.concurrent instrumentation? It uses otel.trace.executors and otel.trace.executors.all properties.

what do u think of

  • otel.instrumentation.executor.include
  • otel.instrumentation.executor.include.all

and renaming java-concurrent module to executor? (and we could rename java-classloader to simply classloader, I don't think either of those terms requires java- prefix for clarification)

There are also otel.trace.methods, otel.trace.annotations and otel.trace.annotated.methods.exclude that are used in external annotations instrumentation

maybe

  • otel.instrumentation.methods.include
  • otel.instrumentation.annotations.include
  • otel.instrumentation.annotations.methods.exclude

and splitting external-annotations into two modules, annotations and methods?

@mateuszrzeszutek
Copy link
Member

That's a great idea!

Two more things to add:

  • Looks like otel.trace.annotated.methods.exclude is also used in the methods instrumentations - we should probably add a dedicated otel.instrumentation.methods.exclude instead.
  • This property is also used in the @WithSpan annotation instrumentation. I'm wondering whether we need this functionality there at all - it's a separate InstrumentationModule, you can turn the whole thing off if you don't need spans from the annotation. Well, if we do need it we can always rename the property.

@iNikem
Copy link
Contributor

iNikem commented Nov 16, 2020

  • This property is also used in the @WithSpan annotation instrumentation. I'm wondering whether we need this functionality there at all - it's a separate InstrumentationModule, you can turn the whole thing off if you don't need spans from the annotation. Well, if we do need it we can always rename the property.

The main use for method exclusion here is to silence some over-zealous library, which has too many WithSpan sprinkled around. You still want this functionality as a whole, but not in that particular part of your application.

@mateuszrzeszutek
Copy link
Member

The main use for method exclusion here is to silence some over-zealous library, which has too many WithSpan sprinkled around.

Good point. Then, what do you think about moving the @WithSpan instrumentation to the new annotations module? It does not really have much in common with the otel-api instrumentation, it suits the purpose of the annotations module and it makes sense to reuse the otel.instrumentation.annotations.methods.exclude property in it.

@trask
Copy link
Member Author

trask commented Nov 17, 2020

I split @WithSpan out of the annotations module in #929 😅

I think the only reason was because it needed the same shading applied as opentelemetry-api.

But that's not a reason it needs to stay there, we could apply the same shading inside of the annotations module too.

I sort of like giving official annotation their own module, what do you think of

annotations module for @WithSpan, with property:

  • otel.instrumentation.annotations.methods.exclude

external-annotations module for others, with properties:

  • otel.instrumentation.external-annotations.include
  • otel.instrumentation.external-annotations.methods.exclude

@mateuszrzeszutek
Copy link
Member

I split @WithSpan out of the annotations module in #929 😅

And now it comes full circle 😂

I sort of like giving official annotation their own module

👍

Okay, to sum up:

  • rename java-concurrent module into executor;
  • move TraceConfigInstrumentationModule from external-annotations to a separate new methods module;
  • move @WithSpan into a new annotations module;
  • and rename all properties so that they match their new module names.

Have I missed anything?

@trask
Copy link
Member Author

trask commented Nov 17, 2020

that sounds good

@trask
Copy link
Member Author

trask commented Nov 22, 2020

updated proposal --

executors

  • otel.instrumentation.executors.include
  • otel.instrumentation.executors.includeAll

methods

  • otel.instrumentation.methods.include

opentelemetry-annotations

  • otel.instrumentation.opentelemetry-annotations.excludeMethods

external-annotations

  • otel.instrumentation.external-annotations.include
  • otel.instrumentation.external-annotations.excludeMethods

classloaders

comments --

Uses plural for all of these module names (executors, classloaders, methods, annotations), because I think it captures the idea that these are broad instrumentations that covers many executors, classloaders, methods, annotations, and also I think the property names looks better, e.g. otel.instrumentation.methods.include vs otel.instrumentation.method.include

Uses "single word" for property name, e.g. otel.instrumentation.executors.includeAll instead of otel.instrumentation.executors.include.all. Thinking maybe(?) this is a good convention, as that part of the name is not really hierarchical, it's the parts before it that define the namespace.

Uses opentelemetry-annotations instead of above annotations, which felt too general, especially when comparing to methods, executors, etc.

@trask
Copy link
Member Author

trask commented Nov 24, 2020

I realized one issue with camelCase is that, at least with our current env var mapping,

otel.instrumentation.executors.includeAll

doesn't map to:

OTEL_INSTRUMENTATION_EXECUTORS_INCLUDE_ALL

but instead maps to

OTEL_INSTRUMENTATION_EXECUTORS_INCLUDEALL

So maybe otel.instrumentation.executors.include-all?

@trask
Copy link
Member Author

trask commented Nov 24, 2020

Copying comment here from #1747 (comment) in favor of kebab-case:

Spring Boot env var / system property mappings supports both camelCase and kebab-case, but recommends kebab-case:

Property Note
acme.my-project.person.first-name Kebab case, which is recommended for use in .properties and .yml files.

And also interestingly:

The prefix value for the annotation must be in kebab case (lowercase and separated by -, such as acme.my-project.person).

So I'm thinking kebab case is probably the way to go (and as a benefit is consistent with our module names when used in the property names, e.g. otel.instrumentation.external-annotations.exclude-methods).

@mateuszrzeszutek
Copy link
Member

I think that we can close this issue - @trask has already updated all instrumentations to use the otel.instrumentation.{instrumentation-name}.separate-words-with-dashes format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants