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

ddtrace/ext: add additional tags for peers and databases #300

Merged
merged 6 commits into from
Aug 8, 2018
Merged

Conversation

dd-caleb
Copy link
Contributor

@dd-caleb dd-caleb commented Aug 6, 2018

This pulls out the ext changes from: #294 and uses them in database/sql.

@dd-caleb dd-caleb added apm:ecosystem contrib/* related feature requests or bugs feature-request labels Aug 6, 2018
@dd-caleb dd-caleb added this to the next milestone Aug 6, 2018
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

My intention with this package was to keep constants pointing to keys and values that have special (or standardized) meaning at Datadog. I was hoping it would be a place where users could go for documentation and to learn more about various significant tags.

The constants in this PR are not used by us nor are they standard and I feel keeping them will create extra noise and confusion. Unless we're planning to make these standard, I'd remove them. We can (and should IMHO) keep only db.user and db.application which we do seem to automatically use with SQL integrations.

@dd-caleb
Copy link
Contributor Author

dd-caleb commented Aug 8, 2018

I'm not familiar with the standards. I copied them from Java open-tracing, which we used in dd-trace-java. I will remove all but db.user and db.application.

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Thanks Caleb.

@@ -44,7 +44,7 @@ func TestMySQL(t *testing.T) {
ext.SpanType: ext.SpanTypeSQL,
ext.TargetHost: "127.0.0.1",
ext.TargetPort: "3306",
"db.user": "test",
ext.DBUser: "test",
"db.name": "test",
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're at it... Should we add this one too? Seems only logical.

@dd-caleb dd-caleb merged commit dbdb82b into v1 Aug 8, 2018
@dd-caleb dd-caleb deleted the caleb/ext branch August 8, 2018 14:18
@gbbr gbbr modified the milestones: next, 1.2.0 Aug 22, 2018
mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants