opentelemetry-erlang icon indicating copy to clipboard operation
opentelemetry-erlang copied to clipboard

Convert attributes to record before storing in ETS for metrics

Open albertored opened this issue 1 year ago • 9 comments

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.

albertored avatar Oct 06 '23 16:10 albertored

Ops, yes sure, I may have missed a couple of "save without formatting"... don't know why the editor is not honouring my settings

albertored avatar Oct 06 '23 17:10 albertored

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!.

codecov[bot] avatar Oct 07 '23 15:10 codecov[bot]

Reverted the format changes

albertored avatar Oct 07 '23 15:10 albertored

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?

tsloughter avatar Oct 13 '23 10:10 tsloughter

That said, I think the limit changes to support separate limits between spans and in general attribute limits is good and necessary.

tsloughter avatar Oct 13 '23 10:10 tsloughter

@albertored any thoughts on my comments?

tsloughter avatar Oct 29 '23 10:10 tsloughter

@tsloughter sorry, busy days, I'll go through comments later today or tomorrow

albertored avatar Oct 29 '23 11:10 albertored

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?

albertored avatar Oct 30 '23 08:10 albertored