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

Log trace data for discarded traces #2816

Merged

Conversation

MarcinGinszt
Copy link
Contributor

@MarcinGinszt MarcinGinszt commented Aug 21, 2023

What this PR does:
This PR adds the RootSpanName and RootServiceName to log about discarded span.
I added them to main log notifying about this event, since those are basic data allowing identification of the span, and this log doesn't occur often (or, atleast, it should not do so 🙂)

I decided to not include test update at least in the first version of PR, as it looks like they would demand significant changes.
In order to simulate the rootSpanName and rootSpanService, we would need to change the MakeBatch function preparing the spans for tests- first span ParentSpanId would have to be empty. However, definition of "empty" is different between different encodings (empty byte array or simply nil).
As far as I understood, because of this issue testing this feature would demand replicating huge chunks of testing library for different encodings, or additional logic changes. Otherwise, multiple tests from different packages would expect nil and would get []byte{} as a value of first span ParentSpanID- or vice versa.

Which issue(s) this PR fixes:
Fixes #2776

Checklist

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

@CLAassistant
Copy link

CLAassistant commented Aug 21, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

I decided to not include test update at least in the first version of PR,

Can you extend this test to make sure it extracts the fields correctly?
https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet3/compactor_test.go#L150

I think that would be worthwhile. I completely agree it's not worth writing a test to make sure a log line gets written as expected.

Thanks for taking a look at this!

tempodb/encoding/vparquet3/block_findtracebyid.go Outdated Show resolved Hide resolved
tempodb/encoding/vparquet3/compactor.go Show resolved Hide resolved
@MarcinGinszt
Copy link
Contributor Author

MarcinGinszt commented Aug 22, 2023

@joe-elliott

Can you extend this test to make sure it extracts the fields correctly?
https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet3/compactor_test.go#L150

I think that would be worthwhile. I completely agree it's not worth writing a test to make sure a log line gets written as expected.

I'm sorry, I'm afraid I don't know how to do that without replicating huge parts of pkg/util/test/req.go, so that batch is created differently for v1 and v3. I wrote detailed description of the problem in the PR description.
Should I proceed with replicating MakeBatch for different versions?

@joe-elliott
Copy link
Member

Should I proceed with replicating MakeBatch for different versions?

Nah, we don't have to do anything complex here.

I'm sorry, I'm afraid I don't know how to do that without replicating huge parts of pkg/util/test/req.go

Is something like this not sufficient:

image

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Nice addition. Thanks!

@joe-elliott joe-elliott merged commit ef29986 into grafana:main Aug 23, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to get trace names of too big traces?
3 participants