Skip to content

fix(contrib/gorm.io): allow resource name to be overridden by custom tags #3909

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
Merged
merged 1 commit into from
Nov 7, 2025
Merged

fix(contrib/gorm.io): allow resource name to be overridden by custom tags #3909

merged 1 commit into from
Nov 7, 2025
+43 −9

Conversation

@will-bem
Copy link
Contributor

@will-bem will-bem commented Aug 21, 2025

What does this PR do?

When using package github.com/DataDog/dd-trace-go/contrib/gorm.io/gorm.v1/v2, I want to override the resource name (because the SQL is long and unwieldy). Alas, I cannot do this with gormtrace.WithCustomTag(ext.ResourceName, ... because the span.SetTag(ext.ResourceName, db.Statement.SQL.String()) call is done after processing custom tags. Thus, I propose moving the custom tag processing to below where the default resource name is set.

Thank you for your consideration!

@will-bem will-bem requested review from a team as code owners August 21, 2025 15:19
darccio
darccio approved these changes Aug 26, 2025
View reviewed changes
Copy link
Member

@darccio darccio left a comment

Choose a reason for hiding this comment

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

LGTM, leaving to @rarguelloF the definitive approval. I wonder if this should be applied to other contribs with this behaviour.

@will-bem will-bem force-pushed the contrib-gorm-resource-name branch 2 times, most recently from f211614 to a1a4af1 Compare August 29, 2025 12:07
@will-bem will-bem force-pushed the contrib-gorm-resource-name branch from a1a4af1 to 4102032 Compare September 19, 2025 13:22
@will-bem
Copy link
Contributor Author

will-bem commented Sep 19, 2025

@rarguelloF Would you mind if I asked for a review of this please? I would love to be able to override the resource name! Thanks for your consideration!

rarguelloF
rarguelloF requested changes Sep 24, 2025
View reviewed changes
Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

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

The changes LGTM, but we should add a test for this change to ensure we don't introduce regressions in the future.

@rarguelloF rarguelloF added the apm:ecosystem contrib/* related feature requests or bugs label Oct 3, 2025
@will-bem will-bem force-pushed the contrib-gorm-resource-name branch from 4102032 to 300e7ee Compare October 31, 2025 20:29
will-bem
will-bem commented Oct 31, 2025
View reviewed changes
contrib/gorm.io/gorm.v1/gorm.go
func withCallbacks(db *gorm.DB, opts ...Option) (*gorm.DB, error) {
cfg := new(config)
defaults(cfg)
cfg := newConfigWithDefaults()
Copy link
Contributor Author

@will-bem will-bem Oct 31, 2025

Choose a reason for hiding this comment

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

the defaults function was only ever used here with a new config, so I thought that this would be an improvement (so that I didn't need do duplicate two lines in my new unit test)

@will-bem
Copy link
Contributor Author

will-bem commented Oct 31, 2025

@rarguelloF I have pushed up a new change which includes a unit test!! 😄 Would you mind taking a look please? 🙏

@will-bem will-bem requested a review from rarguelloF October 31, 2025 21:52
@rarguelloF
Copy link
Contributor

rarguelloF commented Nov 4, 2025

Thanks!! @will-bem

@rarguelloF
Copy link
Contributor

rarguelloF commented Nov 4, 2025

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Nov 4, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-11-04 09:24:19 UTC ℹ️ Start processing command /merge

2025-11-04 09:24:27 UTC ℹ️ MergeQueue: waiting for PR to be ready

This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2025-11-04 13:25:07 UTC ⚠️ MergeQueue: This merge request was unqueued

devflow unqueued this merge request: It did not become mergeable within the expected time

@will-bem will-bem force-pushed the contrib-gorm-resource-name branch from 300e7ee to 211549d Compare November 4, 2025 21:16
@rarguelloF
Copy link
Contributor

rarguelloF commented Nov 5, 2025

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Nov 5, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-11-05 11:03:31 UTC ℹ️ Start processing command /merge

2025-11-05 11:03:37 UTC ℹ️ MergeQueue: waiting for PR to be ready

This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2025-11-05 11:28:22 UTC ℹ️ MergeQueue: merge request added to the queue

The expected merge time in main is approximately 18m (p90).

2025-11-05 11:41:04 UTC ❌ MergeQueue: The checks failed on this merge request

Tests failed on this commit cc1b798:

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.

@rarguelloF
Copy link
Contributor

rarguelloF commented Nov 5, 2025

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Nov 5, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-11-05 14:17:57 UTC ℹ️ Start processing command /merge

2025-11-05 14:18:03 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 18m (p90).

2025-11-05 14:30:53 UTC ❌ MergeQueue: The checks failed on this merge request

Tests failed on this commit 4c56ce0:

What to do next?

  • Investigate the failures and when ready, re-add your pull request to the queue!
  • If your PR checks are green, try to rebase/merge. It might be because the CI run is a bit old.
  • Any question, go check the FAQ.

@rarguelloF
Copy link
Contributor

rarguelloF commented Nov 7, 2025

/merge

@dd-devflow-routing-codex
Copy link

dd-devflow-routing-codex bot commented Nov 7, 2025 •
edited
Loading

View all feedbacks in Devflow UI.

2025-11-07 09:35:01 UTC ℹ️ Start processing command /merge

2025-11-07 09:35:05 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 19m (p90).

2025-11-07 09:47:56 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue dd-mergequeue bot merged commit 2a58e9f into DataDog:main Nov 7, 2025
131 checks passed
@will-bem will-bem deleted the contrib-gorm-resource-name branch November 7, 2025 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@darccio darccio darccio approved these changes

@rarguelloF rarguelloF rarguelloF approved these changes

Assignees

No one assigned

Labels

apm:ecosystem contrib/* related feature requests or bugs mergequeue-status: done

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@will-bem @rarguelloF @darccio