-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Adding data_lifecycle to the _xpack/usage API #96177
Adding data_lifecycle to the _xpack/usage API #96177
Conversation
Hi @masseyke, I've created a changelog YAML for you. |
Pinging @elastic/es-data-management (Team:Data Management) |
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.
Thank you for working on this @masseyke ! I am so glad we will have data so early in the process. I have some comments in the PR, I am curious what you think :)
Also, I was wondering if we could still write a test like x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/health/10_usage.yml
using templates and data stream creation without needing the APIs that might not be available. What do you think?
@@ -545,6 +546,7 @@ public List<NamedWriteableRegistry.Entry> getNamedWriteables() { | |||
), | |||
// Data Streams | |||
new NamedWriteableRegistry.Entry(XPackFeatureSet.Usage.class, XPackField.DATA_STREAMS, DataStreamFeatureSetUsage::new), | |||
new NamedWriteableRegistry.Entry(XPackFeatureSet.Usage.class, XPackField.DATA_LIFECYCLE, DataLifecycleFeatureSetUsage::new), |
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.
We should only register this if the feature flag is on right?
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 wasn't sure -- I always loaded it because the API has an enabled
field. But I guess that enabled field is not actually meant for feature flags, is it? I'll hide all of these if the feature flag is off.
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 think so too.
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/org/elasticsearch/xpack/core/action/DataLifecycleUsageTransportAction.java
Show resolved
Hide resolved
@@ -57,6 +58,7 @@ public class XPackUsageFeatureAction extends ActionType<XPackUsageFeatureRespons | |||
ANALYTICS, | |||
CCR, | |||
DATA_STREAMS, | |||
DATA_LIFECYCLE, |
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.
Behind feature flag?
* This class is meant to test the _xpack/usage API. That API pulls in code from various plugins so it cannot be integration tested | ||
* within x-pack/plugin/core. | ||
*/ | ||
public class XPackUsageIT extends ESRestTestCase { |
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.
Since this is testing only DLM should we make the name a bit more specific?
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 named the test method testDataLifecycle
. But I named the test XPackUsageIT
so that people could add other xpack usage tests here over time. I had hoped to find an XPackUsageIT that I could just add my method to, so I figured someone else might in the future.
On that note though, this is a very expensive test -- it can take about 30 seconds -- and I'm not sure it's actually worth the cost. We have similar (not quite as realistic) coverage in DataLifecycleUsageTransportActionIT (~10 seconds), and serialization coverage in DataLifecycleFeatureSetUsageTests (fast). I'm not sure this test is worth the cost for such simple code with coverage elsewhere.
I had originally intended to do that, but I wanted to add / remove multiple datastreams, and worried that it would turn into a huge mess in yaml. So I added |
I see your concern, I was hoping that with the yaml test we could use the existing spinned up clusters and then just add a few data streams and see we get the correct metrics. Would that be so expensive? It's more of a sanity test not a really detailed one. PS: apologies for the late response. |
I swapped out XPackUsageIT for a yaml rest 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.
LGTM, if I am not mistaken, the transport version in the serialisable object needs to be updated. But it looks good otherwise.
And a small comment about documentation.
...ore/src/main/java/org/elasticsearch/xpack/core/datastreams/DataLifecycleFeatureSetUsage.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/org/elasticsearch/xpack/core/datastreams/DataLifecycleFeatureSetUsage.java
Outdated
Show resolved
Hide resolved
...ore/src/main/java/org/elasticsearch/xpack/core/datastreams/DataLifecycleFeatureSetUsage.java
Outdated
Show resolved
Hide resolved
x-pack/qa/usage/src/main/java/org/elasticsearch/xpack/usage/UsageQA.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Mary Gouseti <[email protected]>
….com:masseyke/elasticsearch into feature/adding-data-lifecycle-to-xpack-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.
LGTM! Thank you for working on this @masseyke!
@masseyke I took the liberty to merge your PR. |
This adds a
data_lifecycle
section to the_xpack/usage
API, giving basic information about data lifecycles in the cluster. The data looks something like: