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

contrib/go-redis: add redis db index tag #1755

Merged
merged 4 commits into from Apr 10, 2023

Conversation

rarguelloF
Copy link
Contributor

@rarguelloF rarguelloF commented Feb 21, 2023

What does this PR do?

Adds db.redis.database_index tag to the following integrations:

  • contrib/go-redis/redis
  • contrib/go-redis/redis.v7
  • contrib/go-redis/redis.v8

Motivation

Unify span tags across tracers.

Describe how to test/QA your changes

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@pr-commenter
Copy link

pr-commenter bot commented Feb 21, 2023

Benchmarks

Comparing candidate commit 6d64c54 in PR branch rarguelloF/go-redis-add-db-index-tag with baseline commit 4ab8cb8 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 0 unstable metrics.

@rarguelloF rarguelloF force-pushed the rarguelloF/go-redis-add-db-index-tag branch 2 times, most recently from d70b2ba to cc9f45a Compare February 21, 2023 13:11
@rarguelloF rarguelloF added this to the Triage milestone Feb 21, 2023
zarirhamza
zarirhamza previously approved these changes Feb 22, 2023
@rarguelloF rarguelloF force-pushed the rarguelloF/go-redis-add-db-index-tag branch from cc9f45a to f7623e6 Compare February 23, 2023 13:14
@rarguelloF rarguelloF marked this pull request as ready for review February 27, 2023 18:01
@rarguelloF rarguelloF requested a review from a team February 27, 2023 18:01
@@ -122,10 +122,11 @@ func (c *Pipeliner) execWithContext(ctx context.Context) ([]redis.Cmder, error)
tracer.ResourceName("redis"),
tracer.Tag(ext.TargetHost, p.host),
tracer.Tag(ext.TargetPort, p.port),
tracer.Tag("out.db", p.db),
tracer.Tag("out.db", strconv.Itoa(p.db)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for the type change here to break an existing customer? (I noticed the other major versions of this already use a string here, it's curious this version doesn't)

Copy link
Contributor Author

@rarguelloF rarguelloF Apr 4, 2023

Choose a reason for hiding this comment

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

out.db should not change in this PR (I just changed where the string conversion is happening, since the new db.redis.database_index is an int).
Please let me know if this clarifies the question or if I missed something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, I missed that the strconv was happening before, just in a different spot. Thanks!

Copy link
Contributor

@ajgajg1134 ajgajg1134 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!

@rarguelloF rarguelloF enabled auto-merge (squash) April 10, 2023 15:37
@rarguelloF rarguelloF merged commit bcb15de into main Apr 10, 2023
15 checks passed
@rarguelloF rarguelloF deleted the rarguelloF/go-redis-add-db-index-tag branch April 10, 2023 15:47
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.

None yet

4 participants