Add support of extra mongodb collstats metrics - #12407
Conversation
stefans-elastic
added
enhancement
🚀 Benchmarks reportTo see the full report comment with |
|
@stefans-elastic not sure in which release this feature ended up in metricbeat but you will have to bump the kibana version to that one. |
There was a problem hiding this comment.
You could add metric_type: gauge and unit: byte. Also field namings could be improved check naming conventions: https://www.elastic.co/guide/en/beats/devguide/current/event-conventions.html
There was a problem hiding this comment.
You could add metric_type: gauge and unit: byte.
is this only for stats.storageSize property?
Also field namings could be improved
don't names have to be the same as they are in beats? Also the names match naming of the fields returned by mongo's collstats command - https://www.mongodb.com/docs/manual/reference/command/collStats/#output
I don't think |
@stefans-elastic Your changes will be able in next minor release of beats i.e., 8.18. So, if you set kibana.version in top-level manifest.yml correctly, then you can also merge it now. This change will only be available when some user installs the 8.18 or above stack; and should choose to install this specific version of package. |
|
Can we add a commit message in the PR description to maintain PR hygiene? |
8.18 release is quite late. |
trying to backport to 8.17 (elastic/beats#42358). I think we should be able to include this in 8.17.2 release |
There was a problem hiding this comment.
From working on #12070 I learned that I should append -next to version (in my understanding it's needed in case stack version isn't released yet)
There was a problem hiding this comment.
Appending the -next version is an approach that can be used to merge a change without publishing a version. This can be used to delay the decision on publishing a package, for example if a set of related changes are introduced on different PRs.
This is not linked to the stack version, a package can be published for an stack version that doesn't exist yet.
There was a problem hiding this comment.
thank you for clarification @jsoriano
I think I don't need -next in this PR in this case, right?
There was a problem hiding this comment.
Looking to the changes, and seeing that they include support for 9.0, I would not use -next, but I would still use a prerelease tag by now, given that 9.0 is not GA yet.
Regarding the support for 9.0, I think you should mention it in the changelog too, and maybe bring to a different PR.
There was a problem hiding this comment.
I've noticed that quite a few packages have || ^9.0.0 in the condition. Correct me if we don't need it in this package
There was a problem hiding this comment.
Why do you need to increase the version from 8.13 to 8.17?
Even if the new fields are only available from 8.17, they probably don't do any harm on 8.13, and this way you can more easily keep maintaining support for versions from 8.13 to 8.17.
Correct me if we don't need it in this package
This will be needed, yes. Though, you can consider splitting this PR in two, one for the new fields, and another one for 9.0 support.
There was a problem hiding this comment.
I've noticed that quite a few packages have
|| ^9.0.0in the condition. Correct me if we don't need it in this package
We are doing a bulk change of adding support of 9.0.0 . So you can remove that from this PR.
There was a problem hiding this comment.
Even if the new fields are only available from 8.17, they probably don't do any harm on 8.13, and this way you can more easily keep maintaining support for versions from 8.13 to 8.17.
@jsoriano : In case we don't upgrade to 8.17.2, we will have new fields which actually don't exist for stack version lower than this but will still be shown in the README of the Integration. The user sitting on 8.13 might then also expect those fields which can't be supported. WDYT ?
There was a problem hiding this comment.
We are doing a bulk change of adding support of 9.0.0 . So you can remove that from this PR.
removed 9.0.0 from the PR
added |
There was a problem hiding this comment.
It's not allowed to publish "next" versions.
I cannot find the documentation, now but it was here the steps previously
https://github.com/elastic/integrations/pull/9796/files#diff-7b9748c4f848852d8b40b331e43f9fd58560e923d065183d0eb3e5916d831c9bL146
Should it be set again to 1.16.0 here (keeping the changelog "-next" entry)? @ishleenk17 @jsoriano
There was a problem hiding this comment.
The same happened in this PR #12407
That new version of the "iis" package was not published, since it is not allowed to publish "-next" versions.
Maybe you would need to update the manifest also for that other package ("iis" package).
WDYT ? @ishleenk17 @jsoriano
There was a problem hiding this comment.
It should be possible to use the -next suffix. This is useful to introduce changes without releasing a package version.
I have reopened https://github.com/elastic/package-storage-infra/issues/394 to review the implementation of the process.
As workaround, yes, I think it should be possible to keep the current version here.
There was a problem hiding this comment.
to clarify: in manifest it should be 1.16.0 and in changelog it should be 1.17.0-next, right?
There was a problem hiding this comment.
Wait, we are trying a quick fix on the storage infra side so these packages are fully ignored and not published.
There was a problem hiding this comment.
If we use next in this PR and merge it. I don't think we will be able to publish any other change to the mongodb package until this change is published.
There are other mongodb changes ongoing in parallel which would be merged soon.
So, IMO lets wait to push this PR until release of 8.17.2.
There was a problem hiding this comment.
I've removed -next
There was a problem hiding this comment.
Why does this change need 8.17.2? It is only adding support for more fields, it should be safe to use in older versions.
There was a problem hiding this comment.
@stefans-elastic : Lets keep the version as 8.13.
We should figure out a way to depict in the fields on how to showcase that a particular set of fields are available only from 8.17.2 onwards.
But the kibana version can remain unchanged.
|
since the PR in beats to backport this to 8.17 has been finally merged I think it is safe to assume the feature is going to be available in 8.17.2. So I think we can go ahead and approve/merge this PR (I'm pretty sure I've addressed all PR comments. If you notice something I haven't addressed - please let me know). WDYT? |
|
💚 Build Succeeded
History
|
|
Package mongodb - 1.17.0 containing this change is available at https://epr.elastic.co/package/mongodb/1.17.0/ |
* add support of extra mongodb collstats metrics * update PR id in changelog.yml * remove stats. from collstats fields declaration * fix mongodb package version and kibana version * remove '-next' from version, remove kibana 9.0.0 from the condition * fix syntax error in changelog.yml * revert kibana vertion change in manifest.yml (back to 8.13.0)
* add support of extra mongodb collstats metrics * update PR id in changelog.yml * remove stats. from collstats fields declaration * fix mongodb package version and kibana version * remove '-next' from version, remove kibana 9.0.0 from the condition * fix syntax error in changelog.yml * revert kibana vertion change in manifest.yml (back to 8.13.0)
7 participants
Proposed commit message
Add support of extra mongodb collstats metrics (size, count, avgObjSize, storageSize, totalIndexSize, totalSize, max, nindexes)
Checklist
changelog.ymlfile.Author's Checklist
How to test this PR locally
Related issues
Screenshots