-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Make kibana/stats metricset work for Stack Monitoring without xpack.enabled flag #21496
Make kibana/stats metricset work for Stack Monitoring without xpack.enabled flag #21496
Conversation
Pinging @elastic/stack-monitoring (Stack monitoring) |
Pinging @elastic/integrations-services (Team:Services) |
This pull request doesn't have a |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Steps errors
Expand to view the steps failures
|
Test | Results |
---|---|
Failed | 0 |
Passed | 2265 |
Skipped | 520 |
Total | 2785 |
I'm seeing this error when running MB:
|
I'm still getting the same error:
|
…/stats_xpack_flag
…/stats_xpack_flag # Conflicts: # metricbeat/module/kibana/stats/stats.go
} | ||
}, | ||
"snapshot": false, | ||
"status": "green" | ||
"status": "green", | ||
"usage": { |
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.
With the recent changes in #22732, I don't think we even need to collect usage
any more. WDYT @chrisronline - safe to omit usage
collection in this PR here?
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.
Yes! Please remove it
if m.XPackEnabled { | ||
m.fetchSettings(r, now) | ||
if err = m.fetchSettings(r); err != nil { | ||
return errors.Wrap(err, "error trying to get settings data from Kibana") |
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.
So previously (before this PR), this metricset would behave as follows:
- if
xpack.enabled: true
was set, it would produce two events in eachFetch
iteration, indexed into.monitoring-kibana-*
, one withtype: kibana_stats
and one withtype: kibana_settings
. - if
xpack.enabled: false
was set, it would produce one event in eachFetch
iteration, indexed intometricbeat-*, corresponding to
type: kibana_stats` (but not actually including that field).
With the changes in this PR, it seems like we would always be producing two events per Fetch
iteration: one corresponding to type: kibana_stats
and one corresponding to type: kibana_settings
. I wonder if now it makes sense to keep the kibana/stats
metricset for just the former and create a new metricset, kibana/settings
, for just the latter? It would be more in line with how other Metricbeat modules work. WDYT?
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.
Feel free to perform this split of metricsets in a follow up PR to the feature branch. For now, if you want to merge the PR with the current implementation, that's fine.
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'll do it in a different PR because I have seen that it requires that I change quite a lot of thing in the base module. I mean, I have to change them anyways but I discovered it now.
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've left a couple of comments/questions on this PR. Besides those, this PR LGTM. So I'm approving it now.
Feel free to address the comments before you merge this PR OR merge it as-is now and address the comments in the feature branch later, as you prefer.
Ready for testing. Check
data.json
andsettings_data.json
for a look of how the events look like now.