Skip to content

Fix to remove some attributes from compare() - #5196

Merged
mdisibio merged 7 commits into
grafana:mainfrom
mdisibio:no-compare-parentid
Jun 3, 2025
Merged

Fix to remove some attributes from compare()#5196
mdisibio merged 7 commits into
grafana:mainfrom
mdisibio:no-compare-parentid

Conversation

@mdisibio

@mdisibio mdisibio commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

What this PR does:
When we added the intrinsic for parent span ID, it got automatically included in selectAll for the compare() function. But it isn't useful and excluding it will save significant cpu/mem.

                                                                      │ before.txt │             after.txt             │
                                                                      │   sec/op   │   sec/op    vs base               │
BackendBlockQueryRange/{_name_!=_nil_}_|_compare({status=error})/5-14   3.353 ± 2%   2.879 ± 3%  -14.12% (p=0.002 n=6)

                                                                      │ before.txt │             after.txt             │
                                                                      │  MB_IO/op  │  MB_IO/op   vs base               │
BackendBlockQueryRange/{_name_!=_nil_}_|_compare({status=error})/5-14   26.21 ± 0%   19.64 ± 0%  -25.07% (p=0.002 n=6)

                                                                      │ before.txt  │           after.txt            │
                                                                      │  spans/op   │  spans/op    vs base           │
BackendBlockQueryRange/{_name_!=_nil_}_|_compare({status=error})/5-14   819.6k ± 0%   819.6k ± 0%  ~ (p=1.000 n=6) ¹
¹ all samples are equal

                                                                      │ before.txt  │             after.txt              │
                                                                      │   spans/s   │   spans/s    vs base               │
BackendBlockQueryRange/{_name_!=_nil_}_|_compare({status=error})/5-14   244.5k ± 2%   284.7k ± 3%  +16.45% (p=0.002 n=6)

                                                                      │  before.txt   │              after.txt               │
                                                                      │     B/op      │     B/op       vs base               │
BackendBlockQueryRange/{_name_!=_nil_}_|_compare({status=error})/5-14   807.4Mi ± 27%   403.0Mi ± 42%  -50.08% (p=0.002 n=6)

                                                                      │ before.txt  │             after.txt              │
                                                                      │  allocs/op  │  allocs/op   vs base               │
BackendBlockQueryRange/{_name_!=_nil_}_|_compare({status=error})/5-14   6.359M ± 3%   2.746M ± 4%  -56.83% (p=0.002 n=6)

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
@joe-elliott

Copy link
Copy Markdown
Collaborator

this may be beyond the scope of your change, but i'm seeing other suspect returned values:

image

and

image

@mdisibio

mdisibio commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

this may be beyond the scope of your change, but i'm seeing other suspect returned values:

The switch statement already excludes nestedSetParent, strange, yeah can take a look.

mdisibio added 3 commits June 2, 2025 14:06
…ix where a nil attribute could be returned from selectall from scopes without any attributes
@mdisibio mdisibio changed the title Exclude parent span id from selectAll Jun 2, 2025
@mdisibio

mdisibio commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

Pushed 2 fixes:

  1. nestedSetParent - This was being returned because it was part of the first pass. Drilldown runs queries like {nestedSetParent<0 && true} | compare({status = error}, 10). So the fix was to exclude grouping on it in the compare() function itself.

  2. instrumentation/resource. unnamed attributes - This was because a nil unnamed attribute was being created by attributeCollector unintentionally, for any scopes that had zero generic attributes. Because if all keys and values are nil, it's encoded as a single nil at the parent definition level. I think this has been around a while, but rare for span and resource, and really first shown after we added the instrumentation scope. There are a couple ways to filter this out, but I think using the skip nils predicate in createAttributeIterator is the best.

@mdisibio
mdisibio merged commit e0b0f25 into grafana:main Jun 3, 2025
mdisibio added a commit to mdisibio/tempo that referenced this pull request Jun 3, 2025
* Exclude parent span id from selectAll

* update test

* Fix to ignore nestedSetParent and other intrinisics from compare(). Fix where a nil attribute could be returned from selectall from scopes without any attributes

* changelog

* lint

* woops lint again
mdisibio added a commit that referenced this pull request Jun 3, 2025
* Exclude parent span id from selectAll

* update test

* Fix to ignore nestedSetParent and other intrinisics from compare(). Fix where a nil attribute could be returned from selectall from scopes without any attributes

* changelog

* lint

* woops lint again
mattdurham pushed a commit to mattdurham/tempo that referenced this pull request Jun 18, 2026
* Exclude parent span id from selectAll

* update test

* Fix to ignore nestedSetParent and other intrinisics from compare(). Fix where a nil attribute could be returned from selectall from scopes without any attributes

* changelog

* lint

* woops lint again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants