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

Add analytics_sample_rate to all integrations #697

Merged
merged 27 commits into from
Mar 12, 2019

Conversation

delner
Copy link
Contributor

@delner delner commented Feb 22, 2019

Using the feature defined in #665, and as a follow up to #666, this pull request adds the event sample rate tag, if configured, to the following integrations:

@delner delner added integrations Involves tracing integrations do-not-merge/WIP Not ready for merge feature Involves a product feature labels Feb 22, 2019
@delner delner added this to the 0.20.0 milestone Feb 22, 2019
@delner delner self-assigned this Feb 22, 2019
@delner delner requested a review from brettlangdon February 22, 2019 19:17
@delner delner force-pushed the feature/add_event_sample_rate_to_all_integrations branch 5 times, most recently from 6bcb655 to 00f099b Compare February 28, 2019 20:25
@brettlangdon
Copy link
Member

We want to use analytics_sample_rate instead of event_sample_rate.

There should be 1 global configuration option analytics when set to true will enable all analytics for all web frameworks only (rack, rails, sinatra, etc)

Each integration will then have it's own 2 configuration options:

  • analytics - boolean, whether to explicitly enable or disable for the integration (default: nil so we can test if it was explicitly set or not)
  • analytics_sample_rate - float, the sample rate to use for that integration (default: 1.0)

For web frameworks, we should set the tag with analytics_sample_rate if the (global[:analytics] == true && integration[:analytics] != false) or integration[:analytics] == true. Global is true and integration is nil or true or the integration is true.

For everything else, we should set the tag with analytics_sample_rate if integration[:analytics] == true

@delner delner force-pushed the feature/add_event_sample_rate_to_all_integrations branch from e4022ef to f2793f6 Compare March 5, 2019 02:58
@delner
Copy link
Contributor Author

delner commented Mar 5, 2019

@brettlangdon All of the existing commits have been updated to match the name changes and logic, with the exception of the global flag (which does not yet exist.) I will carry on updating the remaining integrations from here.

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

these changes look good so far!

@delner delner changed the title Add event_sample_rate to all integrations Add analyics_sample_rate to all integrations Mar 7, 2019
@delner delner changed the title Add analyics_sample_rate to all integrations Add analytics_sample_rate to all integrations Mar 7, 2019
@brettlangdon brettlangdon modified the milestones: 0.20.0, 0.21.0 Mar 8, 2019
@delner delner force-pushed the feature/add_event_sample_rate_to_all_integrations branch 2 times, most recently from d308cc9 to 4d372c6 Compare March 8, 2019 19:53
@delner delner changed the base branch from 0.20-dev to 0.21-dev March 8, 2019 19:53
@delner delner force-pushed the feature/add_event_sample_rate_to_all_integrations branch from 4d372c6 to 21764ad Compare March 8, 2019 20:54
@delner delner removed the do-not-merge/WIP Not ready for merge label Mar 8, 2019
@delner
Copy link
Contributor Author

delner commented Mar 8, 2019

@brettlangdon Ready for review. Worth pointing out Rails, Sinatra and all the other integrations in this PR are off by default, and will ignore the global flag unless explicitly enabled. Let me know if you'd like to change that behavior.

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

We probably only want the request spans to have analytics and can remove setting the tag for rendering spans.

Otherwise, this is good to go!

@delner delner force-pushed the feature/add_event_sample_rate_to_all_integrations branch from ba644e9 to 34ae074 Compare March 11, 2019 18:59
@delner delner force-pushed the feature/add_event_sample_rate_to_all_integrations branch from 34ae074 to b7660eb Compare March 11, 2019 19:09
@delner
Copy link
Contributor Author

delner commented Mar 11, 2019

@brettlangdon Removed tags from render & cache spans, fixed a constant name, and changed defaults such that only Grape, GraphQL, Rack, Rails, & Sinatra will be toggled by the global flag.

Copy link
Member

@brettlangdon brettlangdon left a comment

Choose a reason for hiding this comment

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

d-(^-^)-b

@delner delner merged commit 0ecbd6e into 0.21-dev Mar 12, 2019
@delner delner deleted the feature/add_event_sample_rate_to_all_integrations branch August 12, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants