Skip to content

Defaults for name and resource on SpanData#923

Merged
SammyK merged 3 commits intomasterfrom
sammyk/span-data-defaults
Jun 18, 2020
Merged

Defaults for name and resource on SpanData#923
SammyK merged 3 commits intomasterfrom
sammyk/span-data-defaults

Conversation

@SammyK
Copy link
Copy Markdown
Contributor

@SammyK SammyK commented Jun 15, 2020

Description

This PR adds default values to DDTrace\SpanData::$name and DDTrace\SpanData::$resource when they are not explicitly set from a tracing closure.

  • SpanData::$name defaults to the fully qualified name of the instrumented call in the form of ClassName.methodName or functionName
  • SpanData::$resource defaults to the value of SpanData::$name

In a future PR SpanData::$service will default to the value of the parent span's SpanData::$service, or DD_SERVICE for top-level spans.

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.

@SammyK SammyK added 🏆 enhancement A new feature or improvement 🍏 core Changes to the core tracing functionality c-extension Apply this label to issues and prs related to the C-extension 📦 Sandbox API labels Jun 15, 2020
@SammyK SammyK added this to the 0.47.0 milestone Jun 15, 2020
@SammyK SammyK force-pushed the sammyk/span-data-defaults branch 5 times, most recently from e56e379 to 3f29692 Compare June 17, 2020 21:04
@SammyK SammyK marked this pull request as ready for review June 17, 2020 21:05
@SammyK SammyK force-pushed the sammyk/span-data-defaults branch from 3f29692 to ce30c04 Compare June 17, 2020 21:10
Comment thread src/ext/php7/serializer.c Outdated
Comment on lines +310 to +314
zval prop_resource_as_string;
if (Z_TYPE_P(prop_resource) != IS_NULL) {
ddtrace_convert_to_string(&prop_resource_as_string, prop_resource);
_add_assoc_zval_copy(el, "resource", &prop_resource_as_string);
zval_dtor(&prop_resource_as_string);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's extract a static function that basically does:

        ddtrace_convert_to_string(&prop_resource_as_string, prop_resource);
        _add_assoc_zval_copy(el, "resource", &prop_resource_as_string);
        zval_dtor(&prop_resource_as_string);

This way we can replace 4 lines with 1 for resource, service, and type.

Copy link
Copy Markdown
Collaborator

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

Looks good, Sammy, let's 🚢!

@SammyK SammyK merged commit b34918a into master Jun 18, 2020
@SammyK SammyK deleted the sammyk/span-data-defaults branch June 18, 2020 13:48
@morrisonlevi morrisonlevi mentioned this pull request Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c-extension Apply this label to issues and prs related to the C-extension 🍏 core Changes to the core tracing functionality 🏆 enhancement A new feature or improvement 📦 Sandbox API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants