-
Notifications
You must be signed in to change notification settings - Fork 262
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
[apm] trace search #485
[apm] trace search #485
Conversation
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.
It looks good to me, but given that I have no experience with Ruby or Puppet, it'd be nice if we could try it out and make sure it's working before merging. Also, CI is failing.
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.
Could we please add some spec tests for the feature? It will give us a good sense of how the code is performing and keep us honest in the future.
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.
There's no A6 configuration management added in the PR, and we're still missing spec tests.
templates/datadog_footer.conf.erb
Outdated
@@ -347,7 +347,14 @@ process_agent_enabled: <%= @process_enabled %> | |||
# Enable the trace agent. | |||
apm_enabled: <%= @apm_enabled %> | |||
|
|||
# Enable and the trace search & analytics by adding a list of entered APM events. | |||
<% if ! @apm_analyzed_spans.nil? -%> |
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 that given the way this has been defined to use types we can probably just do:
<% if @apm_analyzed_spans -%>
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.
Oh yes very good point.
7759a50
to
b03b8a3
Compare
Issues addressed:
Merging. |
* Added the ability to apm events for trace search * [apm] adding apm_analyzed_spans for A6 * [apm] adding support for APM analyze spans in A5 + changing argument to hash * [readme] docs + broken table + init description
Corrected PR origin now submitting again.
Missing Spec test code, would like some help with that if possible. I have added and updated the manifest, param and init.pp files with the ability to automate the trace search & analytics configuration in the datadog.yaml file.