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.

Sign up for GitHub

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

Jump to bottom

Extend distributor trace logger with optional features to include span attributes and filter by error status #1465

Merged
merged 17 commits into from
Jun 13, 2022
Merged

Extend distributor trace logger with optional features to include span attributes and filter by error status #1465

merged 17 commits into from
Jun 13, 2022

Conversation

faustodavid
Copy link
Contributor

@faustodavid faustodavid commented Jun 1, 2022 •
edited
Loading

What this PR does:
Extend distributor trace logger with optional features to include span attributes and filter by error status

This can help to do error correlation analysis by attribute.

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

Checklist

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

@faustodavid
Extend ditributor trace logger with optional feature to include the s…
71a12e0 
…pan attributes and to filter by status error

Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
format dependencies in test file
ef11e2c 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
update CHANGELOG.md
c7ad66b 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
Copy link
Contributor Author

Hey @kvrhdn, in my last draft PR, you mentioned making log_received_traces backward compatible. Do you have an example of how I can parse the YAML optionally as a bool or struct?

Also the name log_received_traces, I think it should be log_received_spans, what do you think?

@faustodavid
apply lint
fd1605b 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
update docs, update e2e tests
d5bff1b 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
make LogReceivedTraces backward compatible, update docs
8de8a6d 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
fix config file
a35b22f 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
add optional log_received_spans, update tests to test resources attri…
fed55e4 
…butes, update docs

Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@kvrhdn
Copy link
Member

kvrhdn commented Jun 2, 2022

Hey @kvrhdn, in my last draft PR, you mentioned making log_received_traces backward compatible. Do you have an example of how I can parse the YAML optionally as a bool or struct?

Also the name log_received_traces, I think it should be log_received_spans, what do you think?

I think you can do this by implementing the Unmarshaler interface: this gives you more control over how yaml is converted into your Go struct.
That said, I like the idea to switch to log_received_spans: it's more accurate and it avoids the issue of having to be backwards compatible.

@faustodavid
update integrations tests to use new log_received_spans
0c9361f 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
test LogReceivedTraces flow, remove blank space in doc
0b9ea06 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
use optional spans parameter in makeInstrumentationLibrary to simplif…
0affacf 
…y tests

Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
use assert.EqualValues instead of reflect.DeepEqual
4b5a84d 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
fix typo in test
521b5ff 
Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid faustodavid marked this pull request as ready for review June 2, 2022 15:35
mapno
mapno reviewed Jun 3, 2022
View reviewed changes
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

General approach looks good. I left to comments about some code structure. Thanks for the contribution!

modules/distributor/config.go Outdated Show resolved Hide resolved
modules/distributor/distributor.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
kvrhdn
kvrhdn reviewed Jun 3, 2022
View reviewed changes
Copy link
Member

@kvrhdn kvrhdn left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice work on the tests! Works as expected in my local environment :)

modules/distributor/config.go Outdated Show resolved Hide resolved
faustodavid and others added 2 commits June 5, 2022 17:52
@faustodavid
* Split logSpanWithAttribute into smaller functions.
abd6e43 
* Update CHANGELOG with deprecated flag

Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid @kvrhdn
Update modules/distributor/config.go
a9cdc08 
Co-authored-by: Koenraad Verheyden <[email protected]>
knylander-grafana
knylander-grafana approved these changes Jun 10, 2022
View reviewed changes
Copy link
Contributor

@knylander-grafana knylander-grafana left a comment

Choose a reason for hiding this comment

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

Doc updates look good. I fixed a capitalization error on another line.

mapno
mapno reviewed Jun 10, 2022
View reviewed changes
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'm in favor of merging when the last two minor threads are closed 👍

@faustodavid
* Rename include_attributes to include_all_attributes
fb8366c 
* Add warning log when log_received_traces flag is use

Signed-off-by: Fausto David Suarez Rosario <[email protected]>
@faustodavid
Copy link
Contributor Author

faustodavid commented Jun 10, 2022 •
edited
Loading

Hey @mapno @kvrhdn , I have linked an issue to the PR to describe a bit better the intention of this feature. #1485

mapno
mapno approved these changes Jun 13, 2022
View reviewed changes
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

Great work 😄. Thanks for the contribution!

@mapno mapno merged commit da138b1 into grafana:main Jun 13, 2022
@faustodavid faustodavid mentioned this pull request Aug 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvrhdn kvrhdn kvrhdn left review comments

@mapno mapno mapno approved these changes

@knylander-grafana knylander-grafana knylander-grafana approved these changes

@KMiller-Grafana KMiller-Grafana Awaiting requested review from KMiller-Grafana

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner

@annanay25 annanay25 Awaiting requested review from annanay25

@mdisibio mdisibio Awaiting requested review from mdisibio mdisibio is a code owner

@dgzlopes dgzlopes Awaiting requested review from dgzlopes

@zalegrala zalegrala Awaiting requested review from zalegrala zalegrala is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enrich log_received_traces with attributes and error filter
4 participants
@faustodavid @kvrhdn @mapno @knylander-grafana