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

Log delta count in addition to throughput in LoggingMeterRegistry #5548

Open
fstaudt opened this issue Oct 2, 2024 · 4 comments · May be fixed by #5590
Open

Log delta count in addition to throughput in LoggingMeterRegistry #5548

fstaudt opened this issue Oct 2, 2024 · 4 comments · May be fixed by #5590
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Milestone

Comments

@fstaudt
Copy link

fstaudt commented Oct 2, 2024

Please describe the feature request.
In LoggingMeterRegistry, instruments such as counter, timer and histogram are logged only with a throughput (e.g. throughput=0.016667/s).
I propose to add the count next to the throughput in the logs for those instruments (e.g. throughput=0.016667/s count=1).

Rationale
For instruments such as counter, timer and histogram, throughput is not always the most useful information to log.
In some cases, users want to know the count as it is exported by other meter registries (e.g. OTLP).

Deriving the count from the throughput requires to know the step and it is not always obvious from logs (especially when logInactive is disabled).
Adding the count in the logs next to the throughput would make the logs more clear.

Additional context
N/A

I can provide a PR if it is OK for you.

@shakuzen
Copy link
Member

shakuzen commented Oct 3, 2024

In some cases, users want to know the count as it is exported by other meter registries (e.g. OTLP).

The count exported by other registries depends on what is expected with that registry. Some use a cumulative count, some use a delta count, some report each increment, some might use a step-normalized throughput. Since LoggingMeterRegistry is a StepMeterRegistry its meters are step meters, which for count means delta counts. But if you're trying to compare it with PrometheusMeterRegistry, for example, that is going to have cumulative counts instead. If we made it clear the logged count is a step count (delta count), that might help alleviate confusion, but it would still be hard to meaningfully compare it with other registries that aren't also a normal StepMeterRegistry. Ultimately, I'm not sure using the LoggingMeterRegistry as a means to tell what is exported by other registries is the best approach. Some registries themselves have logging that can be enabled to log the payload sent to the backend, if that's the level of debugging desired. I guess it might be helpful to understand more the intended use case here for the LoggingMeterRegistry. That said, I'm not opposed to showing a count, but we would want to take the above into consideration and not encourage people to use the LoggingMeterRegistry for some purpose it isn't fit for.

@shakuzen shakuzen added waiting for feedback We need additional information before we can continue and removed waiting-for-triage labels Oct 3, 2024
@fstaudt
Copy link
Author

fstaudt commented Oct 3, 2024

Ultimately, I'm not sure using the LoggingMeterRegistry as a means to tell what is exported by other registries is the best approach. Some registries themselves have logging that can be enabled to log the payload sent to the backend, if that's the level of debugging desired.

In our use case, we want to use:

  • OtlpMeterRegistry for PROD environments that sends metrics to OpenTelemetry collector (itself sending metrics to Dynatrace).
    OtlpMeterRegistry is configured to produce delta metrics as required by Dynatrace.
  • LoggingMeterRegistry for DEV environments and local workstations where Dynatrace is not used.

We considered logging at the level of OpenTelemetry collector for DEV environments but we wanted to avoid extra network costs between applications and collector.
Moreover, metrics logged in collector aggregate metrics from all applications in cluster and it makes it very complex for developers to find the metric they are interested in.

We don't plan to use LoggingMeterRegistry to compare metrics with other registries, I agree that it is better to have logging capabilities in each registry (if needed).
Our goal is only to have similar numbers in both approaches.

If we made it clear the logged count is a step count (delta count), that might help alleviate confusion

I also agree that using delta_count=1 instead of just count=1 will make it more clear that the count displayed in logs is not cumulative.

@shakuzen
Copy link
Member

shakuzen commented Oct 4, 2024

Sounds reasonable to me. Would you like to make a pull request for it?

@shakuzen shakuzen added enhancement A general enhancement and removed waiting for feedback We need additional information before we can continue labels Oct 4, 2024
@shakuzen shakuzen added this to the 1.x milestone Oct 4, 2024
@shakuzen shakuzen added the module: micrometer-core An issue that is related to our core module label Oct 8, 2024
@fstaudt
Copy link
Author

fstaudt commented Oct 8, 2024

Sounds reasonable to me. Would you like to make a pull request for it?

Yes I will.

Thanks

@fstaudt fstaudt changed the title Log count in addition to throughput in LoggingMeterRegistry Log delta count in addition to throughput in LoggingMeterRegistry Oct 14, 2024
fstaudt pushed a commit to fstaudt/micrometer that referenced this issue Oct 14, 2024
fstaudt pushed a commit to fstaudt/micrometer that referenced this issue Oct 14, 2024
fstaudt pushed a commit to fstaudt/micrometer that referenced this issue Oct 15, 2024
@shakuzen shakuzen modified the milestones: 1.x, 1.15.0-M1 Oct 16, 2024
fstaudt pushed a commit to fstaudt/micrometer that referenced this issue Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants