-
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
[Metricbeat] Kibana/stats set exclude_usage=true
in the init
phase
#29911
Conversation
This pull request does not have a backport label. Could you fix it @afharo? 🙏
NOTE: |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
…ana/stats/exclude_usage
Pinging @elastic/stack-monitoring (Stack monitoring) |
Pinging @elastic/integrations-services (Team:Services) |
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.
Makes sense to me! Maybe good to get @sayden to weigh-in
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.
Please, double check the thing I commented. I also suggest to test with both metricsets at the same time, again, just to double check. Every metricset uses a different http client based on the metricset but I'd prefer to confirm this before the storm of SDH's appear.
// Add exclude_usage=true if the Kibana Version supports it | ||
if kibana.IsUsageExcludable(kibanaVersion) { | ||
origURI := statsHTTP.GetURI() | ||
statsHTTP.SetURI(origURI + "&exclude_usage=true") |
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 double check this change. The code in this part is definitely obscure but if the defer
statement is deleted here, it might be the case that the uri will be &exclude_usage=true&exclude_usage=true&exclude_usage=true
after 3 periods.
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 makes perfect sense! Is there any documentation I can use to run this PR? Or any artifacts generated during CI that I can use to test?
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.
Nope sorry, you'll have to do a go build
and setup everything 😅
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 wait you can use https://github.com/elastic/elastic-package to spin up the entire environment and then use go build
with your code.
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.
@sayden I spent a while trying to test it locally, but I'm on an M1 Mac now and there's a library that is no-longer compatible. I'd appreciate some help when you have some time 😇
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 have just seen this. I'll do a try asap
This pull request is now in conflicts. Could you fix it? 🙏
|
@sayden @matschaffer what should we do with this PR? |
@klacabane you should be aware of this |
I opened #31118 to have this reviewed and possibly merged. |
Closing in favour of the new approach discussed in #31118. |
What does this PR do?
It moves the full URL generation to the
init
phase for the Kibana-stats collection module.Why is it important?
Since
m.isUsageExcludable
is calculated in theinit
phase, I think that it's not necessary to overwrite it on everyfetch
.Also, because of the previous
defer
statement, when building the event later on, it showed the original URL instead of the one actually performed (including&exclude_usage=true
).Checklist
- [x] I have made corresponding changes to the documentation- [x] I have made corresponding change to the default configuration filesCHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
init
method multiple times? Can this be an issue when calling theGetVersion
method?How to test this PR locally
Related issues
Use cases
Screenshots
Logs