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

adding WithCustomTag to a few libs in contrib/: gorm/gorm.v1, confluentinc/confluent-kafka-go/kafka, database/sql, google.golang.org/grpc #1309

Closed
wants to merge 18 commits into from

Conversation

duxing
Copy link

@duxing duxing commented May 25, 2022

support custom tags for a few libs in contrib/:

  • gorm.io/gorm.v1
  • confluentinc/confluent-kafka-go/kafka
  • database/sql
  • google.golang.org/grpc

Fixes #1308

support custom tags for gorm.v1 instrumentation

Fixes DataDog#1308
@duxing duxing requested a review from a team May 25, 2022 23:22
@ajgajg1134 ajgajg1134 added this to the 1.39.0 milestone Jun 1, 2022
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.

Hi duxing! Thanks for this contribution, it looks like a good idea to bring both these versions to feature parity. Just one small comment I've attached, and also can you add a unit test for this change? You may be able to borrow nearly all the test from the previous version which has a test TestCustomTags that covers this.

Thanks again!

contrib/gorm.io/gorm.v1/option.go Show resolved Hide resolved
support custom tags for confluent-kafka-go/kafka instrumentation
support custom tags for confluent-kafka-go/kafka instrumentation
support custom tags for google.golang.org/grpc instrumentation
@duxing duxing changed the title contrib/gorm.io/gorm.v1: adding WithCustomTag adding WithCustomTag to a few libs in contrib/: gorm.io/gorm.v1, confluentinc/confluent-kafka-go/kafka, database/sql, google.golang.org/grpc Jun 16, 2022
@duxing duxing changed the title adding WithCustomTag to a few libs in contrib/: gorm.io/gorm.v1, confluentinc/confluent-kafka-go/kafka, database/sql, google.golang.org/grpc adding WithCustomTag to a few libs in contrib/: jinzhu/gorm, confluentinc/confluent-kafka-go/kafka, database/sql, google.golang.org/grpc Jun 20, 2022
@duxing duxing changed the title adding WithCustomTag to a few libs in contrib/: jinzhu/gorm, confluentinc/confluent-kafka-go/kafka, database/sql, google.golang.org/grpc adding WithCustomTag to a few libs in contrib/: gorm/gorm.v1, confluentinc/confluent-kafka-go/kafka, database/sql, google.golang.org/grpc Jun 20, 2022
@duxing duxing requested a review from ajgajg1134 June 21, 2022 08:24
@duxing
Copy link
Author

duxing commented Jun 21, 2022

hi @ajgajg1134 sorry about the delay. I was able to get to this last night and added test coverage for everything I introduced. can you take another look when you have a chance?

I noticed the CI fails complaining about DD_API_KEY missing in the env (and another bitnami/kafka related issue). I'm not sure how to address these ones, it'll be helpful if you can share some insights with me

@duxing
Copy link
Author

duxing commented Jun 27, 2022

hi @ajgajg1134 can you help me understand why my PR fails due to:

Neither DATADOG_API_KEY nor DD_API_KEY is in your environment.
Internal Error: API key is missing
when other PRs (eg. this and this ) don't have the problem? it's probably something trivial and I'd like to figure out what I missed.

thanks in advance!

@ajgajg1134
Copy link
Contributor

hi @ajgajg1134 can you help me understand why my PR fails due to:

Neither DATADOG_API_KEY nor DD_API_KEY is in your environment.
Internal Error: API key is missing
when other PRs (eg. this and this ) don't have the problem? it's probably something trivial and I'd like to figure out what I missed.

thanks in advance!

Hello @duxing! Sorry, the system-test failures you're seeing are due to this PR running a branch from an external fork of the repository. Unfortunately it means these tests can't be run unless the branch is coming from within the repository, although we can run the other tests still and should be able to merge after review is complete. (This is a limitation of github actions that hopefully is resolved at some point)

@duxing
Copy link
Author

duxing commented Jun 27, 2022

@ajgajg1134 thx for explaining, i figured it has something to do with the github action settings. I've disabled my own fork since it's complaining about the same thing.

is it possible to cherry-pick my change to a branch for this repo and proceed?

@Hellzy Hellzy modified the milestones: 1.39.0, 1.40.0 Jul 1, 2022
@Hellzy Hellzy modified the milestones: 1.40.0, 1.41.0 Jul 15, 2022
@ajgajg1134
Copy link
Contributor

ajgajg1134 commented Aug 12, 2022

Closing this PR since the work was merged with #1359
Thanks for the contribution!

@ajgajg1134 ajgajg1134 closed this Aug 12, 2022
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.

Request: example for custom tags for contrib/gorm.io
4 participants