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

[Feature]: Provide direct access to a counter, i.e. without need for key/value. #2115

Closed
etep opened this issue Sep 13, 2024 · 4 comments
Closed
Labels
enhancement New feature or request triage:todo Needs to be traiged.

Comments

@etep
Copy link

etep commented Sep 13, 2024

Describe the solution you'd like:

In all the examples, incrementing a counter requires that the user supply a key value pair. This requires the counter to do a key value lookup to find the correct count to increment. What is expected is that there is a value somewhere in memory and the count is added to that (maybe atomically). What we have is a complicated code path to do some lookup followed by a simple increment. See, e.g., this.

Is it possible to directly access the "add" or "increment" method for a metric, i.e. without key/value lookup?

Related:

1740.

@etep etep added enhancement New feature or request triage:todo Needs to be traiged. labels Sep 13, 2024
@cijothomas
Copy link
Member

I guess what you are after is the bound instrument idea, which is something we expect to add post the 1.0 stable release.
#1374

If you don't have any key-value pairs, then pass an empty slice - we have internally optimized for that scenario so there won't be any lookup/hashes and it'll be super fast (like 10-20 nano secs only)! But if you need key-values, then it is inevitable that there be some lookup in an in-memory structure, until we support bound instruments!

@etep
Copy link
Author

etep commented Sep 13, 2024

Thank you -- and, yes, #1374 looks right. If there's a request here, then it amounts to a mention in the examples or docs. It's surprising to me that the "bound" use case is not the default. To my thinking, the bound use case is:

  • easier to understand, closer match to "naive" or "default" expectations.
  • faster.

Also, I was digging in and found that the empty slice option existed. I think it should be in the examples. Agreed, it is optimized for, but I would expect the bound version of things to eliminate a little more overhead.

@cijothomas
Copy link
Member

The "default" can indeed be quite subjective! From a typical OpenTelemetry perspective, I'd say bound instruments are often considered a niche scenario. Here's why:

Bound instruments require knowing both key and value ahead of time, which isn't always available.
Even when available, there could be many combinations (potentially 100s or 1000s), forcing users to figure out how to store them. I've seen users attempting to put these in their own hashmap (guarded by Locks), causing the same performance issues bound instruments were originally designed to avoid! 😅

OpenTelemetry has a high focus on HTTP-based apps, where metric dimensions like "route", "HTTP verb", and "status_code" are common, and they aren't an ideal fit for bound instruments. This is one reason why bound instruments was removed from the v1 OTel Metric spec to keep the scope manageable. We're currently discussing reintroducing bound instruments to the spec. It'll take some time to write, prototype, and stabilize the spec across multiple languages. However, we understand that Rust isn't limited to web apps, and there are many scenarios where OTel Rust would benefit from bound instruments. We're aiming to add support soon after the 1.0 stable release, without being entirely constrained by spec velocity.

For those coming from Windows performance counters, network packet counting, or similar domains, bound instruments might feel more familiar. These users are accustomed to the ultra-low cost (1-5 nanoseconds) of simply incrementing a number for metrics. For them, OTel's typical range of 100s of nanoseconds can be quite a shift.
(If you're curious, the 100s ns range is after a major rewrite we completed just recently. Previously, it was in the microseconds range. The old benchmarks are still available here. I'm about to update them with the current numbers, which should show a 10x improvement!)

(We'll add more docs are we gear towards stable, will definitely include the 0 attribute spl case. Thanks for sharing your feedbacks)

@cijothomas
Copy link
Member

I guess this issue can be closed in favor of the existing one #1374

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triage:todo Needs to be traiged.
Projects
None yet
Development

No branches or pull requests

2 participants