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

Move span resource name to method #811

Merged
merged 3 commits into from
Oct 1, 2019
Merged

Conversation

giancarlocosta
Copy link
Contributor

Name span resource via method, similar to the service_name method.

@giancarlocosta giancarlocosta requested a review from a team September 10, 2019 17:22
@marcotc
Copy link
Member

marcotc commented Sep 10, 2019

Looks good @giancarlocosta!
CI seems to be failing because of linting errors. Would you mind running rubocop on your branch and addressing any issues?

You'll likely get a green build after that 👍

@marcotc marcotc added community Was opened by a community member dev/refactor Involves refactoring existing components labels Sep 10, 2019
@giancarlocosta
Copy link
Contributor Author

@marcotc Thanks! Looks good now except for deploy prelease Gem. Anything I need to do for that?

@marcotc
Copy link
Member

marcotc commented Sep 10, 2019

@giancarlocosta Don't worry about deploy prerelease Gem, it currently only works for external contributors.

@marcotc
Copy link
Member

marcotc commented Sep 10, 2019

Just one small thing: we want our PRs to be based on the latest unreleased version branch, which currently is 0.28-dev.
Would you mind rebasing this PR into 0.28-dev instead of master?

@giancarlocosta giancarlocosta changed the base branch from master to 0.28-dev September 10, 2019 20:34
@@ -72,6 +72,10 @@ def service_name(env)
options[:service_name]
end

def resource_name(env)
Copy link
Contributor

Choose a reason for hiding this comment

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

@giancarlocosta I think this seems totally fine: what's the motivation for doing so? I imagine you want to patch/override this to customize resource names?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delner Yeah that's the goal! 👍

Copy link
Contributor

@delner delner Sep 11, 2019

Choose a reason for hiding this comment

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

@giancarlocosta Okay, if that's the case, I think that's totally fine, but keep in mind that this component is not considered a part of the public API and therefore might be subject to change in the future without warning. Just want to make sure that risk is understood and acceptable given such a change might break your instrumentation.

@giancarlocosta
Copy link
Contributor Author

@delner @marcotc Thanks guys! Do you have an estimate of when you'll be able to merge this? I looked on the guidelines but didn't see anything about merge times

@delner
Copy link
Contributor

delner commented Sep 25, 2019

@giancarlocosta We'll consider this for our next minor release, which usually happen on Tuesdays. We will sometimes skip a Tuesday if we don't think we have enough substantial changes to ship.

@marcotc marcotc merged commit e2ce082 into DataDog:0.28-dev Oct 1, 2019
@marcotc marcotc added this to the 0.28.0 milestone Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member dev/refactor Involves refactoring existing components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants