-
Notifications
You must be signed in to change notification settings - Fork 435
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
profiler: add profiler package #598
Conversation
This change simply copies over the private Go profiler into the `profiler` package of `dd-trace-go`.
This change removes the option of adding a custom logger and instead uses the already existing logger from dd-trace-go which will prefix messages with `Datadog Tracer %version%: %message%`
This change temporarily removes environment variables until we decide for a standardized naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! We just need to sort out WithService[Name] and as far as I can tell we can merge.
cada2b5
to
4404084
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go once we hear back on WithService[Name]!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo!
This change adds `WithService` to the tracer as an alias for `WithServiceName` which now becomes marked as deprecated. It also renames `WithServiceName` to `WithService` in the `profiler` package. Additionally, these are added to the example, along with the version tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really cool, @gbbr.
} | ||
} | ||
|
||
// WithTags specifies a set of tags to be attached to the profiler. These may help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extremely minor suggestion
// WithTags specifies a set of tags to be attached to the profiler. These may help | |
// WithTags specifies a set of tags to be attached to the profiles. These may help |
func TestDefaultConfig(t *testing.T) { | ||
t.Run("base", func(t *testing.T) { | ||
cfg := defaultConfig() | ||
assert := assert.New(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some places we use assert := assert.New(t)
and some places we pass t
for every call to assert
.
Is there a good reason to do one over the other in different places? If not I think it looks more consistent to stick with one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine where it's just one line of code, e.g. assert.True(t, ok)
vs adding more code which defines a variable only to be used once. Arguably there's places where it's used more than once and could be changed...
}, nil | ||
} | ||
|
||
func mutexProfile(cfg *config) (*profile, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heapProfile
, cpuProfile
, blockProfile
, and mutexProfile
look almost identical except for some string values and the profile function that is called.
What do you think about unifying them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example of what that'd look like? We may want to reconsider after trying to put it together 🙂I prefer verbosity if it adds to simplicity.
This change adds support for Datadog Profiling. The `profiler` package is used as a standalone package and must be started separately from the tracer. Because it is part of the same repository, it will be imported using the same domain: ``` import "gopkg.in/DataDog/dd-trace-go.v1/profiler" ``` To start the profiler, make sure you pass it your Datadog API Key: ``` if err := profiler.Start(profiler.WithAPIKey("123key")); err != nil { log.Fatalf("Error starting profiler: %v", err) } defer profiler.Stop() ``` Various other options may be used and can be seen in the documentation: http://godoc.org/gopkg.in/DataDog/dd-trace-go.v1/profiler/
This change adds support for Datadog Profiling.
The
profiler
package is used as a standalone package and must be started separately from the tracer. Because it is part of the same repository, it will be imported using the same domain:To start the profiler, make sure you pass it your Datadog API Key:
Various other options may be used and can be seen in the documentation: http://godoc.org/gopkg.in/DataDog/dd-trace-go.v1/profiler/