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.
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
Refactor state_* metricsets to share response from endpoint #25640
Refactor state_* metricsets to share response from endpoint #25640
Changes from 6 commits
5d82b5d
0f38774
34b6b10
92cdae2
6d7a325
2d7f70b
84ad39f
7ef7056
fd87273
befc83d
8c7d267
6aa439f
7656de5
3f099a0
a12bb15
f52cb66
6a4b0e7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is a race condition here. If this function is called by two metricsets at the same time you could end up with several fetchers running.
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.
Also, what if different metricsets have a different period?
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.
Period is defined at the module level, so all metricsets will have the same period.
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.
Both cases should be handled by https://github.com/elastic/beats/pull/25640/files#diff-9933f42aec81125910c9923f83fa89c9a76918aac6e3bd5a68c430aeeab91084R98-R106.
Taking into consideration that period is set on Module's level then I think that different period's check is redundant. Metricsets will not share the input if they are configured in different Modules like:
So the period adjustment at https://github.com/elastic/beats/pull/25640/files#diff-9933f42aec81125910c9923f83fa89c9a76918aac6e3bd5a68c430aeeab91084R100 could be removed.
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.
Take into account that this configuration will instantiate two different modules, and they won't reuse the shared families. If we want/need to supports configs like these ones, this approach won't work. To reuse the families we would need one of:
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.
Can you investigate what Agent is doing with a config like that? I think doing this at the module level is good enough if the Agent handles it correctly, and removes the complexity of indexing by endpoint, metrics path and other parameters (including the security part).