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

ddtrace/tracer: implement fmt.Formatter interface to support log injection #657

Merged
merged 9 commits into from
May 15, 2020

Conversation

knusbaum
Copy link
Contributor

@knusbaum knusbaum commented May 6, 2020

This change implements fmt.Formatter to allow printing a span in various formats for logging.

…ction

This change implements `fmt.Formatter` to allow printing a span in various formats for logging.
@knusbaum knusbaum added this to the 1.24.0 milestone May 6, 2020
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
@knusbaum knusbaum requested a review from gbbr May 6, 2020 14:52
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_test.go Show resolved Hide resolved
@knusbaum knusbaum requested a review from jdgumz May 6, 2020 15:23
jdgumz
jdgumz previously approved these changes May 6, 2020
Copy link
Contributor

@jdgumz jdgumz left a comment

Choose a reason for hiding this comment

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

LGTM on last comment for tests.

ddtrace/tracer/span_test.go Outdated Show resolved Hide resolved
jdgumz
jdgumz previously approved these changes May 6, 2020
@knusbaum knusbaum modified the milestones: 1.24.0, 1.25.0 May 6, 2020
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

I think we need to do our best to find the information needed in a log, even if the tracer might be stopped. If the tracer is absent, we can still get a:

  • Service by checking globalconfig.ServiceName()
  • Env by checking DD_ENV
  • Version by checking DD_VERSION

We shouldn't give up as easily by leaving them empty.

ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/option_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/option_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Show resolved Hide resolved
ddtrace/tracer/span_test.go Outdated Show resolved Hide resolved
@knusbaum
Copy link
Contributor Author

knusbaum commented May 7, 2020

Hmmm. I'm not sure how best to implement picking up env, version, and service from WithGlobalTag and DD_TAGS.
WithGlobalTag options get called after defaults() are set and env vars are picked up, but should only set version, env, and service in certain cases.

i.e.
with DD_TAGS == version:1.2.3, and WithGlobalTag("version", "2.3.4"), WithGlobalTag should take precedence over DD_TAGS and version should be 2.3.4

But
With DD_VERSION == 1.2.3, and WithGlobalTag("version", "2.3.4"), WithGlobalTag should not take precedence, and version should be 1.2.3.

Currently there's no way for WithGlobalTag to know whether version was set from DD_TAGS or from DD_VERSION.

@gbbr
Copy link
Contributor

gbbr commented May 7, 2020

Hmmm. I'm not sure how best to implement picking up env, version, and service from WithGlobalTag and DD_TAGS.

If you mean specifically in the fallback when no tracer is started, in that case ignore these. It's just a fallback. 99.9% of the time this won't happen. Plus, tags are not the right place to set service, env and version.

@knusbaum knusbaum requested review from jdgumz and gbbr May 8, 2020 21:03
@knusbaum
Copy link
Contributor Author

knusbaum commented May 8, 2020

Sorry for causing confusion here. I have deleted some of my comments around DD_TAGS to reduce noise.

I will put up another PR around picking up env, version, and service from DD_TAGS and WithGlobalTag and we can discuss it there.

This PR handles only the logging of env, version, and service.

@knusbaum knusbaum merged commit f58fd8c into v1 May 15, 2020
@knusbaum knusbaum deleted the knusbaum/formatter branch July 14, 2020 16:28
mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
…ction (DataDog#657)

This change implements `fmt.Formatter` to allow printing a span in various formats for logging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants