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

Uri to resource name ON by default #798

Merged
merged 15 commits into from
Apr 6, 2020
Merged

Conversation

labbati
Copy link
Member

@labbati labbati commented Mar 17, 2020

Description

This PR enable the feature request URI by default. In addition to that a few changes were necessary to test real web request with different env variables.

Readiness checklist

  • [*] (only for Members) Changelog has been added to the appropriate release draft. Create one if necessary.
  • [*] Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the appropriate release draft. For community contributors the reviewer is in charge of this task.

@labbati labbati added this to the 0.43.0 milestone Mar 17, 2020
Copy link
Contributor

@SammyK SammyK left a comment

Choose a reason for hiding this comment

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

Looks great @labbati! Just one little tweak and this should be good to go. :)

@@ -384,6 +388,10 @@ public function getTracesAsArray()
$globalTags = $this->globalConfig->getGlobalTags();
if ($globalTags) {
foreach ($internalSpans as &$internalSpan) {
// If resource is empty, we normalize it the the operation name.
if ($internalSpan['resource'] === null) {
$internalSpan['resource'] === $internalSpan['name'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this is supposed to be an assignment, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well yes, and interestingly enough this hasn't been caught by tests, this means at least one more test is required or the exiting ones have to be modified. Thanks for noticing

Copy link
Member Author

@labbati labbati Mar 18, 2020

Choose a reason for hiding this comment

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

Fix to this comment.

I was just wondering....why you need a test suite when you have @SammyK doing your code reviews? 😎

@@ -70,4 +72,121 @@ public function testGlobalTagsArePresentOnInternalSpansByFlushTime()
public function dummyMethod()
{
}

/**
* When resource is set through tracer's $config object, it should be honored for CLI
Copy link
Contributor

Choose a reason for hiding this comment

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

The extra comments are 🔝

@labbati labbati force-pushed the labbati/uri-resource-on-default branch from 4f1f873 to d8ea222 Compare March 18, 2020 09:59
SammyK
SammyK previously approved these changes Mar 26, 2020
Copy link
Contributor

@SammyK SammyK left a comment

Choose a reason for hiding this comment

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

Nice work @labbati! :)

SammyK
SammyK previously approved these changes Mar 30, 2020
@labbati labbati modified the milestones: 0.42.0, 0.43.0 Mar 30, 2020
@labbati labbati added the 🏆 enhancement A new feature or improvement label Apr 2, 2020
@labbati
Copy link
Member Author

labbati commented Apr 2, 2020

The failing test is the usual flaky on 5.4

Comment on lines 22 to 28
protected function tearUp()
{
\putenv('DD_TRACE_GLOBAL_TAGS');
\putenv('DD_TRACE_URL_AS_RESOURCE_NAMES_ENABLED');
parent::tearDown();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm suspicious here. Why is tearUp calling parent::tearDown? Shouldn't this be setUp or tearDown?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, tearUp should be tearDown for sure. Good catch. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch 😄! The test is not failing because in this case I was doing a putenv at the begin of relevant tests that do override that value. Nonetheless, it is a good habit to left the env clean after every test.

@labbati labbati merged commit e323228 into master Apr 6, 2020
@labbati labbati deleted the labbati/uri-resource-on-default branch April 6, 2020 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏆 enhancement A new feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants