Skip to content

[SERF-3423] Adding MySQL driver instrumentation #112

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

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

laynax
Copy link
Contributor

@laynax laynax commented Jun 20, 2024

Description

This PR adds MySQL driver tracing using dd-trace-go library.

Testing considerations

Tested on document-search staging.

Sample span:
Screenshot 2024-06-20 at 17 46 55

Checklist

  • Prefixed the PR title with the JIRA ticket code
  • Performed simple, atomic commits with [good commit messages][commit messages]
  • Verified that the commit history is linear and commits are squashed as necessary
  • Thoroughly tested the changes in development and/or staging
  • Updated the README.md as necessary

Related links

@laynax laynax force-pushed the laynax/SERF-3423/trace-driver branch 2 times, most recently from fc2cd39 to 68f5a6f Compare June 20, 2024 15:27
@laynax laynax marked this pull request as ready for review June 20, 2024 15:48
@laynax laynax requested a review from a team as a code owner June 20, 2024 15:48
@laynax laynax requested a review from fotos June 20, 2024 15:48
@laynax laynax force-pushed the laynax/SERF-3423/trace-driver branch from 68f5a6f to fc38460 Compare June 20, 2024 15:52
fotos
fotos previously approved these changes Jun 21, 2024
Copy link
Contributor

@fotos fotos left a comment

Choose a reason for hiding this comment

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

👍 LGTM 🚀

@laynax laynax force-pushed the laynax/SERF-3423/trace-driver branch from 1653149 to 0e2d409 Compare June 21, 2024 13:38
@laynax laynax requested a review from fotos June 21, 2024 13:51
@laynax laynax force-pushed the laynax/SERF-3423/trace-driver branch 3 times, most recently from 7896ed7 to d3b9e5c Compare June 21, 2024 14:43
Comment on lines 36 to 37
dbServiceName := fmt.Sprintf("%s-mysql", appName)
driverServiceName := fmt.Sprintf("%s-orm", appName)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think those need to be the other way around? 🤔

The (MySQL) driver is %s-mysql and the db (ORM) is %s-orm.

Suggested change
dbServiceName := fmt.Sprintf("%s-mysql", appName)
driverServiceName := fmt.Sprintf("%s-orm", appName)
dbServiceName := fmt.Sprintf("%s-orm", appName)
driverServiceName := fmt.Sprintf("%s-mysql", appName)

Copy link
Contributor Author

@laynax laynax Jun 21, 2024

Choose a reason for hiding this comment

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

Current value for dbServiceName is with -mysql, this way we will have document-search-orm instead.
Maybe change driverServiceName to appname-dbdriver?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @fotos about the naming: the -orm should be assigned to the gorm instrumentation, and the -mysql to the MySQL driver instrumentation.

@laynax If we change the name for the gORM instrumentation, what happens? I think the only thing will change is the DataDog metric names, which we will need to update anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fotos @Neurostep

Changed as requested 👍

Overall I'm against having more than one service name for each service, it must be the operation name providing info about orm or mysql type.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fotos A brief update here. After discussing this PR with @laynax we decided to keep the service name as is %s-mysql (document-search-mysql) as it will make things easier and effectively a part of one database flow. One can distinguish between different span types using the operation type drop-down (see screenshot). Please, let us know what you think about this 🙏

Screenshot 2024-06-28 at 10 34 05

@laynax Let's update the service name here using -mysql suffix for both clients 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

Let's see how that works in practice. 👍

@laynax laynax requested a review from fotos June 21, 2024 16:20
@laynax laynax force-pushed the laynax/SERF-3423/trace-driver branch from d3b9e5c to dc5db8c Compare June 26, 2024 09:51
@laynax laynax requested a review from Neurostep June 26, 2024 09:58
@laynax laynax force-pushed the laynax/SERF-3423/trace-driver branch from dc5db8c to fc38460 Compare June 28, 2024 09:08
Copy link
Contributor

@fotos fotos left a comment

Choose a reason for hiding this comment

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

👍 LGTM 🚀

Copy link
Contributor

@Neurostep Neurostep left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@laynax laynax merged commit 6670be7 into main Jun 28, 2024
@laynax laynax deleted the laynax/SERF-3423/trace-driver branch June 28, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants