Comments (4)
After talking with Cijo a bit, we came to the conclusion to revert the precached attribute set work, pause the bound instruments API, and add the concept of instrument level attributes at instrument (e.g. counter) creation time. I'll investigate what that will look like.
from opentelemetry-rust.
Adding some more context:
- To the question if this is a real requirement or not -- Yes this is a core principle of OTel. NoOp APIs allow library authors to bet on OTel API without worrying that their library users will pay perf cost even if they are not ultimately using OTel. The biggest ask from library authors trying to natively instrument their libraries are about "how much cost will be paid, if my end users does not enable telemetry".
- Also, ability to swap out official SDK with a custom one, is a requirement - that, though rare, allows folks to write custom sdks meeting perf needs not normally possible with official ones. But this can only be feasible if API delegates every work to the SDK. (if we use
tracing
as an example - the APIs are near zero cost in the absence of a listener, and listeners can swapped out based on perf needs. This principle is same in Otel, though terminology used is different.) - Making
AttributeSet
a trait in API and allowing SDK to provide implementation is a feasible route - need to benchmark and see if the dynamic dispatch cost is acceptable. Until we do this, it is better to revert theAttributeSet
changes.
The idea of adding attributes at Instrument creation time, and then providing nothing more at the measurement reporting time, can allow the SDK to offer a very fast Metrics SDK, for those scenarios where the attributes are known ahead of time (as in the case @KallDrexx is primarily after.) This is similar to bound-instrument concept, but there is no need of strictly providing a bound-instrument API, as it is possible to achieve this with instrument level attributes.
counter = meter.CreateCounterWithAttributes(name,descp, attributes);
counter.Add(1); // note - nothing is being passed as attributes, though it is feasible to do so.
In the SDK, counter.Add(1) (measurements with no attributes) can be implemented very efficiently as there is no need to spend time on attributes (which we know are a huge overhead), and there is no need of hashmap lookup (as we can special case the 0-attribute case).
This does not prohibit future re-addition of AttributeSet or addition of a more formal Bound instrument idea - just starting with the above, so as to unblock some high-perf scenarios. (A significant amount of efforts already went into AttributeSet/Bound - we hope they are not totally wasted. we should be able to leverage them in the future)
from opentelemetry-rust.
This feels like an arbitrary requirement to me. How many real users are
- Using opentelemetry-rust with the No-op implementation
- Using it in a performance critical system where the impact is noticeable.
Is this just a theoretical concern or has this really been a legitimate issue in the past?
We could make the AttributeSet
lazily create the InnerAttributeSet
only when actually called upon. This would require either a Mutex
or RwLock
to ensure we can handle concurrent access to the attribute set, which would incur extra costs. It will also require some complex lifetime management, because we can no longer consume the Into<AttributeSet>
iterator immediately, and thus a &[KeyValue]
needs to live long enough until the instantiation.
We can remove precached attributes and only support bounded instruments. This means you still pay the cost of creation when a single attributeset can be used for multiple instruments. This isn't theoretical, for example every packet we receive uses the same attribute set for the "bytes received" counter and the "packet count" counter. This means you can't do an LRU cache of attribute sets, so if your web server handles a request from a tenant it can cache tenant specific attribute sets assuming the load balancer will probably send another request for that tenant to the server again.
We could make AttributeSet
a trait with the current implementation in the SDK. This would require annotating everything with a second generic trait, which may be complex with a lot of the dyn Any
and other dynamic calls there are, at least without extra runtime costs. It also has complexity in passing counters around, because the generic parameter can't be inferred in many circumstances due to it being used in a different area of code than its creation or the measure
call.
from opentelemetry-rust.
SIG call:
- Revert the PR before conflicts start appear. -- Matthew will send PR
- Meter (instrumentation scope attributes) or instrumentation attributes needs to be explored to see if it can indeed achieve the high perf.
* Might need special casing 0 attributes case. - Matt/Cijo will explore this, and send PRs.
* View might complicate things, but maybe not.. need to explore. - Cijo will explore this.
from opentelemetry-rust.
Related Issues (20)
- [Feature]: Expose BatchLogProcessor configuration HOT 3
- [Feature]: Use `opentelemetry-semantic-conventions` internally. HOT 2
- [Bug]: The `reqwest` feature also tries to configure `reqwest::blocking::Client` and it might not be available
- [Feature]: Possibly update Extractor::key(&self) signature.
- [Bug]: SDK Trace propagator documentation is hidden.
- [Feature]: Avoid parsing empty strings into `TraceId`
- [Feature]: Update HTTP dependencie to ^1.0.0 HOT 1
- [Bug]: `SimpleSpanProcessor` might fail and error is ignored HOT 2
- [Bug]: opentelemetry-appender-tracing not setting log timestamp HOT 4
- There should be a way to create a synchronous guage. HOT 2
- Remove aws id generator from core sdk HOT 4
- Flaky test HOT 10
- [Bug]: incorrect docs on SdkMeterProvider.builder() HOT 2
- [Bug]: The datadog and dynatrace crates referenced in README.md are published in crates.io but repo references 404
- Flaky test in CI HOT 4
- [Bug]: PeriodicReader uses sync::Mutex.
- [Bug]: constructing a tonic builder defaults to connect to 4318? HOT 6
- Add Testing for Shutdown Scenarios. HOT 4
- ObservableCounter aggregation bug for cumulative
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from opentelemetry-rust.