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

Use bitwise combination for metrics options #456

Closed
wants to merge 3 commits into from

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Jun 24, 2022

Exploring option improvement after discussion here #431 (comment)

Changes

Use bitwise combination for metrics options.

Pro:

  • More granular control on every single metrics.

Con:

  • A ton of options to be maintained. Not much to worry about if using default config, which is enabling all flags.
  • Old issue still remains: it might be counterintuitive to have .AddRuntimeMetrics() as enabling all options as default, and after adding an option, say .AddRuntimeMetrics(options => { options.GcEnabled = true; }), all options other than GcEnabled will be disabled.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #

@xiang17 xiang17 requested a review from a team June 24, 2022 22:04
@xiang17 xiang17 changed the title [WIP] Use bitwise combination for metrics options Use bitwise combination for metrics options Jun 25, 2022
@reyang
Copy link
Member

reyang commented Jun 25, 2022

This seems over complicated, especially considering some metrics' availability is dependent on the runtime version.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can simply start without any options - e.g. AddRuntimeInstrumentation() will just give you everything that's available to the current runtime/platform. Later if we want to introduce options, we can, and that's not a breaking change.

@xiang17
Copy link
Contributor Author

xiang17 commented Jun 25, 2022

I wonder if we can simply start without any options - e.g. AddRuntimeInstrumentation() will just give you everything that's available to the current runtime/platform. Later if we want to introduce options, we can, and that's not a breaking change.

Do you mean remove all the options?

In the current implementation, that's the default behavior if you don't specify any options: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.Runtime#step-2-enable-runtime-instrumentation-at-application-startup

@reyang
Copy link
Member

reyang commented Jun 25, 2022

I wonder if we can simply start without any options - e.g. AddRuntimeInstrumentation() will just give you everything that's available to the current runtime/platform. Later if we want to introduce options, we can, and that's not a breaking change.

Do you mean remove all the options?

In the current implementation, that's the default behavior if you don't specify any options: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.Runtime#step-2-enable-runtime-instrumentation-at-application-startup

I asked this because I saw ActiveTimerCount under the threading options. I don't know if Timer has a direct relation to threading despite the fact that .NET grouped it under the System.Threading namespace.

Overall I think it could take time to design these options and it requires a good understanding of usage scenario. I feel a good starting point is to have nothing configurable.

@cijothomas
Copy link
Member

Agree to have no configurability in the 1st release. Adding the turn off flag later is not a breaking change.

@reyang reyang added the comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime label Jun 25, 2022
@xiang17 xiang17 closed this Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants