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

Attempt to optimise attribute name collision checks. #1328

Merged
merged 9 commits into from
Aug 19, 2024

Conversation

jsuereth
Copy link
Contributor

@jsuereth jsuereth commented Aug 8, 2024

Attribute collision checks were taking >40s to run. This drops it to <1s.

cc @lmolkova

@jsuereth jsuereth added the Skip Changelog Label to skip the changelog check label Aug 8, 2024
@jsuereth jsuereth requested review from a team August 8, 2024 19:30
Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

image

@jsuereth
Copy link
Contributor Author

jsuereth commented Aug 8, 2024

Don't be too excited :) I broke a bit - doing more testing to fix it.

@jsuereth
Copy link
Contributor Author

jsuereth commented Aug 8, 2024

Actually going to close this -> Need to spend some more time

@jsuereth jsuereth closed this Aug 8, 2024
@jsuereth jsuereth reopened this Aug 8, 2024
@jsuereth
Copy link
Contributor Author

jsuereth commented Aug 8, 2024

OK - so I was luckier than I thought.

I was doing something bad with sets, but I went a different route, unfortunately this only takes it form 40s -> 10s, but at least it's an improvement.

@lmolkova
Copy link
Contributor

lmolkova commented Aug 8, 2024

I was doing something bad with sets, but I went a different route, unfortunately this only takes it form 40s -> 10s, but at least it's an improvement.

I have a vague memory of @lquerel doing some optimization for it in weaver 0.8.0, but we are still on 0.7.0.

@lquerel
Copy link
Contributor

lquerel commented Aug 8, 2024

@lmolkova and @jsuereth IIRC this version is faster (~6s on my laptop in debug mode) -> https://github.com/open-telemetry/weaver/blob/main/test_data/attribute_name_collisions.rego

There is a similar approach in this directory for attribute const collision.

BTW, we should build the Docker image in release mode to achieve much faster results in our CI/CD pipeline. The initial issue we had with release mode a while back is now fixed, I believe.

@jsuereth
Copy link
Contributor Author

jsuereth commented Aug 9, 2024

@lquerel Looks like we basically did the same optimisation if you look at the details.

I think we can merge this one if we're comfortable with it, or I can migrate it to look more like yours, as this dramatically reduces time spent just from the first optimisation of pre-computing the namespace/const name mappings.

I'll also take some time to update weaver's docker build to release mode as that should just help overall.

@jsuereth
Copy link
Contributor Author

jsuereth commented Aug 9, 2024

Tested this with optimised docker image build and it drops to 1.5s for the check

@jsuereth
Copy link
Contributor Author

jsuereth commented Aug 9, 2024

I updated to have @lquerel's collision detection as well, on debug build this is:

> Total execution time: 4.745763013s

Optimised, ~1s, so major improvements.

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

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

LGTM

@lmolkova lmolkova merged commit f7d30b1 into open-telemetry:main Aug 19, 2024
12 of 13 checks passed
@jsuereth jsuereth deleted the wip-speed-up-policies branch August 19, 2024 16:01
ezimuel pushed a commit to ezimuel/semantic-conventions that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Label to skip the changelog check
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants