Replies: 18 comments
-
moving this over to the opentelemetry-java project |
Beta Was this translation helpful? Give feedback.
-
Hi @kvcache . The purpose of the |
Beta Was this translation helpful? Give feedback.
-
I think maybe that's missing the point? When I implemented literally this a few years ago I expressed it as allowing a zero-allocation attribute set - rather than exclusively forcing it. It's an arbitrary requirement, not driven by technical necessity, and ought to be called out in docs but allowed. Sometimes you need to do both. Maybe the right compromise here would be to have a means to tell an Instrument or a Consider this silly scope timer, which cannot disambiguate by any dynamic tags, but at least a user doesn't have to pass in Resource attributes at every single report site:
Now, if the Meter could be set to report its Resource Attributes, this would no longer need to be bound and I could pass in optional Timer Attributes to make this way more ergonomic. Also that would be a performance boost as Resource Attributes can be considered immutable and invariant, whereas the bound attribute set is not invariant across instances. (as a kludge I make multiple bound timers with the same name and literal same description with a different value for a single "operation" tag to time the same thing by different facets) |
Beta Was this translation helpful? Give feedback.
-
@jsuereth I wonder if we should remove bound instruments altogether at this point, since they're not specced that I could see in the metrics API spec. |
Beta Was this translation helpful? Give feedback.
-
We could just remove them. Bound instruments are designed (right now) to avoid a single lock within the metrics SDK and can improve performance of writing metrics by 2x. What you're suggesting is to remove that performance benefit. Specifically bound instruments ONLY exist as a performance optimisation when attributes are known at allocation time. I can dig out the benchmarking code if you'd like to see the boost. This feature would basically negate any benefit of bound instruments, instead you should just allocate Attributes and then make append a quick thing (if |
Beta Was this translation helpful? Give feedback.
-
What code where you using here? How much of this is lack of efficient attribute-append operation? |
Beta Was this translation helpful? Give feedback.
-
I can give you an example but not from my actual service:
An efficient append attribute operation would be helpful and would make recording |
Beta Was this translation helpful? Give feedback.
-
static-unchanging-resource attributes should be done as part of Your example here looks like it's not the OTEL API but some other API. Did you check the performance of the OTEL library? Additionally, it looks like your instantiating a list of your labels in every call. If you pre-allocate the label list, would that be more efficient? In otel our API takes |
Beta Was this translation helpful? Give feedback.
-
Yeah, I agree - however, the
Scroll up a bit, it's a class I wrote using the OTEL API to make the OTEL API more ergonomic.
It has not presented a problem yet. The profiler showed its overhead was acceptable.
Nope, it's preallocated! I would like to be able to provide both preallocated-static, and dynamic labels.
Oh I see what you're saying, yeah - I just omitted the AttributesBuilder dance for sake of brevity. There are a bunch of builders and unrelated stuff that distracts from the main issue here: I'm looking to have both |
Beta Was this translation helpful? Give feedback.
-
You can do this if you leverage the Otel collector (e.g. this configuration parameter available on many exporters) to export to prometheus and I think that's a great feature request for the prometheus exporter. SPECIFICALLY, it'd be great if you raised a feature request around this use case (and I can link to that in an OTEL Specification feature request). I'd love to improve the interaction of OTEL Resource + Prometheus Export as I think we can do far better than now, and your use case exactly matches what resource is intended for. E.g. see this related issue: open-telemetry/opentelemetry-specification#1782
I see. I think we're likely to provide a timer instrument in the future. Two main points:
That's not the purpose of the bound API, unfortunately. It's just there for when you know your attributes ahead of time and we can avoid a lock. It also degrades performance when you're leveraging LIke @jkwatson It might be we just drop it to lead to less confusion. I still think it's useful for internal instrumentation to provide the "zero hot-path allocation" guarantees. |
Beta Was this translation helpful? Give feedback.
-
Re: bound instruments
I missed that this was a feature of bound instruments - please do not remove these unless you have an equivalent feature elsewhere in your api, and thank you for educating me on this semantic! I will keep the Timer wrapper then anyway to make the attribute binding more clean. Please know that I am currently depending on bound Instruments and if they are contributing to my observed (very) low garbage collector activity then I will sorely miss their removal. Tbh I will go as far as to fork the repo to keep the feature 😬 but would prefer not. Powerful tools are worth confusion at first glance; let's keep bound instruments in the API if they're the performant option (which I now understand they are). I implore you: Keep bound instruments! Re: Exporters and
|
Beta Was this translation helpful? Give feedback.
-
@jsuereth Do you think you can push a PR to the spec for bound instruments? I'm a bit worried about going with them in one language because they do seem to correlate back to semantic conventions. In particular, I think in most if not all cases, a bound attribute could be encoded into the metric name. We already see this, e.g. http_client_duration and http_server_duration, where client/server could otherwise be a bound attribute. free_mem_heap, free_mem_direct also come to mind as similar. I think it would be unfortunate if we have a fairly large API that may not actually be reflected into semantic conventions (all potential bound attributes end up in the metric name instead) and think we should avoid it. We could consider removing now and adding later if we don't want to go through the spec process for now but think it's important to do. |
Beta Was this translation helpful? Give feedback.
-
The piece of the specification we had added for bound instruments is this line:
I think it'd be hard to specify the EXACT semantics of bound instruments in the specification across all languages, but I'm happy to do some kind of "For Java, here are guarantees of using The issue you mention in semantic conventions of metric name vs. attribute is one I actually think goes beyond the notion of "bound", and needs to be addressed irrespective of it. Have you opened a ticket on semantic conventions about whether http_client_duration + http_server_duration should just be http_duration with a set of attributes? |
Beta Was this translation helpful? Give feedback.
-
I think bound attributes are semantically ones that are known at instrument creation time vs record time. This seems language agnostic and the concept can be added to the spec. Languages could potentially skip any particular implementation if they want since it's always possible to merge at record time. But the concept being introduced would allow use from semantic conventions.
I haven't opened an issue since they both seem ok. I think I've used a pattern like what we already have in apps before using Prometheus and didn't have a problem. I can imagine using attribute without a problem too. Indeed it's convention specific whether it's appropriate to use an attribute vs the name usability wise. But without the concept of bound attributes in the spec, I can imagine actively avoiding them for performance reasons even if it is appropriate usability wise. So I think they are related at least somewhat. |
Beta Was this translation helpful? Give feedback.
-
@anuraaga now that the dust is settling in metric reader, I'll see if I can expand on the performance statement in the specification. I still think @jkwatson question of whether we're using confusing terminology applies. I plan to write up a "how to avoid hot path allocation with otel metrics" markdown file and we can determine whether or not the API we expose is the right one. Given expectations of users, "bind" may be seen as pure convenience vs. performance all the time. I'd like to avoid that confusion |
Beta Was this translation helpful? Give feedback.
-
Great, thanks - avoiding allocation is important for us and having that as a concept that OpenTelemetry promotes as an option is incredibly valuable. Hardening it with docs & expectation around "convenience" versus "performance" would help. Since I (on my current project) must choose obsessively low-cost instrumentation over convenience, I will choose performance-oriented paths without regard for the convenience & setting up that expectation would (I think) have headed off my opening this ticket in the first place. Btw in case it's unclear, I don't want what I asked for anymore because I am convinced that this is the performant pattern and that doing what I asked would erode the low cost that bound instruments promote. Thank y'all again for your education on this ❤️ |
Beta Was this translation helpful? Give feedback.
-
I wonder if it would be useful to convert this to a discussion, @kvcache so others can find it and hopefully understand the nuance a bit better. |
Beta Was this translation helpful? Give feedback.
-
@jkwatson agree - I lack the permissions but any maintainer should feel free to do so from my perspective. |
Beta Was this translation helpful? Give feedback.
-
Is your feature request related to a problem? Please describe.
Not all exporters support adding resource Attributes to outgoing Metrics. To get around that in my system I'm eagerly binding all of my Counters and whatnot to the Resource's Attributes. But the
BoundLongCounter
api (and friends) does not have the overload likevoid add(long value, Attributes attributes);
to allow me to optionally attach some observation-specific Attributes.Describe the solution you'd like
I would like the
BoundLongCounter
interface to extendLongCounter
. Or at a minimum add the Attributes overloads fromLongCounter
.Perhaps obvious, but this issue is not limited to
BoundLongCounter
; it is shared by everyBound**
Instrument and the ask extends to all of them. Binding Attributes should not preclude the possibility of having dynamic Attributes (which I do).Describe alternatives you've considered
Beta Was this translation helpful? Give feedback.
All reactions