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

Convert attributes to record before storing in ETS for metrics #633

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

albertored
Copy link
Contributor

@albertored albertored commented Oct 6, 2023

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.

Copy link
Contributor

@bryannaegele bryannaegele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to separate these formatting changes (assuming @tsloughter is good with the style changes) to different PRs? It really clutters seeing what the changes are in these.

@albertored
Copy link
Contributor Author

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
Copy link

codecov bot commented Oct 7, 2023

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%) ⬆️
apps/opentelemetry/src/otel_limits.erl 95.23% <88.88%> (ø)

📢 Thoughts on this report? Let us know!.

@albertored
Copy link
Contributor Author

Reverted the format changes

@tsloughter
Copy link
Member

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
Copy link
Member

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

@tsloughter
Copy link
Member

@albertored any thoughts on my comments?

@albertored
Copy link
Contributor Author

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

@albertored
Copy link
Contributor Author

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?

@tsloughter
Copy link
Member

Oh yea, after having to change the attributes in a datapoint to a phash of the map because of how ETS does map matching in matchspecs we may want to reconsider going with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants