semantic-conventions icon indicating copy to clipboard operation
semantic-conventions copied to clipboard

Attempt to optimise attribute name collision checks.

Open jsuereth opened this issue 1 year ago • 8 comments

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

cc @lmolkova

jsuereth avatar Aug 08 '24 19:08 jsuereth

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

jsuereth avatar Aug 08 '24 20:08 jsuereth

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

jsuereth avatar Aug 08 '24 21:08 jsuereth

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.

jsuereth avatar Aug 08 '24 21:08 jsuereth

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.

lmolkova avatar Aug 08 '24 22:08 lmolkova

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

lquerel avatar Aug 08 '24 23:08 lquerel

@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 avatar Aug 09 '24 12:08 jsuereth

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

jsuereth avatar Aug 09 '24 13:08 jsuereth

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.

jsuereth avatar Aug 09 '24 14:08 jsuereth