-
Notifications
You must be signed in to change notification settings - Fork 399
Telemetry: send events in forked children #5074
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
Open
p-datadog
wants to merge
10
commits into
master
Choose a base branch
from
telemetry-in-children
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+297
−28
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
1074777
Send telemetry in forked children
p fdf0fe9
consolidate attr_readers
p 1e15731
types
p 8ed7634
skip fork tests on jruby
p 8dc1c4a
mark as integration tests
p e8b48ca
fix syntax
p f2e7239
tabs
p d784c1d
Merge branch 'master' into telemetry-in-children
p 1fb050b
go back to reset_ran_once_state_for_tests
p 9541c23
reset initial event once
p File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,12 @@ class AppStarted < Base | |||||
| def initialize(components:) | ||||||
| # To not hold a reference to the component tree, generate | ||||||
| # the event payload here in the constructor. | ||||||
| # | ||||||
| # Important: do not store data that contains (or is derived from) | ||||||
| # the runtime_id oor sequence numbers. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| # This event is reused when a process forks, but in the | ||||||
| # child process the runtime_id would be different and sequence | ||||||
| # number would obviously also be different. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| @configuration = configuration(components.settings, components.agent_settings) | ||||||
| @install_signature = install_signature(components.settings) | ||||||
| @products = products(components) | ||||||
|
|
@@ -30,6 +36,15 @@ def payload | |||||
| } | ||||||
| end | ||||||
|
|
||||||
| # Whether the event is actually the app-started event. | ||||||
| # For the app-started event we follow up by sending | ||||||
| # app-dependencies-loaded, if the event is | ||||||
| # app-client-configuration-change we don't send | ||||||
| # app-dependencies-loaded. | ||||||
| def app_started? | ||||||
| true | ||||||
| end | ||||||
|
|
||||||
| private | ||||||
|
|
||||||
| def products(components) | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 was reviewing the changes in
SynthAppClientConfigurationChangeand I was wondering: do you think it would make sense to check for forking in this class (Component) so that we can change thisifcondition to:This would mean we make no changes to
SynthAppClientConfigurationChange. The changes inSynthAppClientConfigurationChange, the "mode switching", were hard to follow for me, since I wasn't sure why it would need to pretend to be a different event sometimes. Turns out it's because of of thisComponent#startmethod. I was thinking that we could keep all the logic aroundAppStartedvsSynthAppClientConfigurationChangehere.(Btw, in terms of detecting a fork: it either be done here, or borrowed from
worker.forked?, but that's probably a weird dependency)