opentelemetry-erlang
opentelemetry-erlang copied to clipboard
Convert attributes to record before storing in ETS for metrics
As per title with this PR attributes are converted to attributes record before storing them in ETS tables. Instrument record
and add
functions accept both a raw map or an attributes record.
For doing so the support of OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT
and OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT
environment variables (and corresponding app env ones) has been added.
We can now accept attributes records also in the tracing functions but I will do it in a separate PR.
Ops, yes sure, I may have missed a couple of "save without formatting"... don't know why the editor is not honouring my settings
Codecov Report
Attention: 1 lines
in your changes are missing coverage. Please review.
Files | Coverage Δ | |
---|---|---|
apps/opentelemetry/src/opentelemetry_app.erl | 100.00% <100.00%> (ø) |
|
apps/opentelemetry/src/otel_configuration.erl | 79.33% <100.00%> (ø) |
|
apps/opentelemetry/src/otel_span_utils.erl | 90.62% <100.00%> (ø) |
|
apps/opentelemetry_api/src/otel_attributes.erl | 86.95% <100.00%> (+0.59%) |
:arrow_up: |
apps/opentelemetry/src/otel_limits.erl | 95.23% <88.88%> (ø) |
:loudspeaker: Thoughts on this report? Let us know!.
Reverted the format changes
So I'm less sure of this now. We shouldn't need to be storing all the attribute information like limits on each datapoint, right?
Plus this would mean the ets table is keyed on the name and attributes record when it could technically (I know it isn't currently) be the name as attribute values with a separate list of attribute keys -- separating them to make the key and duplicated data smaller?
That said, I think the limit changes to support separate limits between spans and in general attribute limits is good and necessary.
@albertored any thoughts on my comments?
@tsloughter sorry, busy days, I'll go through comments later today or tomorrow
So I'm less sure of this now. We shouldn't need to be storing all the attribute information like limits on each datapoint, right?
Plus this would mean the ets table is keyed on the name and attributes record when it could technically (I know it isn't currently) be the name as attribute values with a separate list of attribute keys -- separating them to make the key and duplicated data smaller?
Yea, your objections are legit. I did it in this way to mimic what is done for tracing where attributes record is stored in the span ets but I understand your points here.
The main reason for this PR is to validate attributes as soon as possible so that all data consumers down the pipeline (for instance exporters) can assume attributes to be already validated. For doing so it is not necessary to store the record in ets, we can also convert attrs to records for validating them and the store only the map representation in the table. What do you think?
If this is the way should we do it also for span ets so that they both behave consistently?