ESQL: Entirely remove META FUNCTIONS #113967
Conversation
This removes the undocumented `META FUNCTIONS` command that emits descriptions for all functions. This shouldn't be used because we never told anyone about it. I'd have preferred if we'd have explicitly documented it as no public or if we'd have left it snapshot-only. But sometimes you make a mistake. I'm hopeful no one is relying on it. It was never reliable and not public.....
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @nik9000, I've created a changelog YAML for you. Note that since this PR is labelled |
|
Hi @nik9000, I've updated the changelog YAML for you. Note that since this PR is labelled |
| KEEP(Keep.class::isInstance), | ||
| RENAME(Rename.class::isInstance), | ||
| META(MetaFunctions.class::isInstance); | ||
| META(p -> false); |
There was a problem hiding this comment.
Let's add a comment why this exists and why it shouldn't be removed.
Actually, can this be removed? I think @astefan or @luigidellaquila know more about feature metrics and could know if old metrics need to be kept around.
There was a problem hiding this comment.
Let me dig more. I had thought removing it would be dangerous for serialization of the metric. I'll look!
There was a problem hiding this comment.
Looking more, I'm not sure why we have the note about the ordering here. It looks like no instances of this class escape a node. We use strings for serializing it. So maybe we're safe here. Can I just delete this?
There was a problem hiding this comment.
Metrics are not serialized between nodes, and the Stats API relies on the metric name, not on the enum ordinal (that is only used during the metric collection phase, to populate a bitset).
I think it's safe to remove, but please wait for @astefan
There was a problem hiding this comment.
LGTM, thanks Nik.
Just wait for @astefan to confirm that the enum entry can be dropped
| KEEP(Keep.class::isInstance), | ||
| RENAME(Rename.class::isInstance), | ||
| META(MetaFunctions.class::isInstance); | ||
| META(p -> false); |
There was a problem hiding this comment.
Metrics are not serialized between nodes, and the Stats API relies on the metric name, not on the enum ordinal (that is only used during the metric collection phase, to populate a bitset).
I think it's safe to remove, but please wait for @astefan
|
I pushed a patch that removes it. The tests seemed ok locally. Absolutely will wait on feedback though. can revert that patch easily enouhg. |
|
@astefan, do you think it's ok to remove the META constant above? If so, I think I should remove the comment about adding things in order because I don't believe it makes a difference here. |
mark-vieira
added
auto-backport
There was a problem hiding this comment.
I think the changes are ok, given that the CI is happy. @luigidellaquila mentioned that the name of the enum is used and he's correct, but there is serialization I think. The serializations is for the name of the metrics in EsqlUsageTransportAction.masterOperation where counters from all nodes are merged together, each Counter instance being a Map<String,Long>. And the individual Counter instances are built in Metrics.counters().
The only additional step I did when adding all this telemetry fun was to update the mappings of the cluster that holds all this data. That repo where the mappings are held and indices created is a blackbox to me and I have no idea what is the impact of removing one of those counters. I am only assuming that the documents that reach those indices will not have a value for meta/show anymore, which should be a problem I guess?
| "Cluster and node setting", | ||
| "Command line tool", | ||
| "CRUD", | ||
| "ES|QL", |
| protected void shouldSkipTest(String testName) throws IOException { | ||
| super.shouldSkipTest(testName); | ||
| assumeTrue("Test " + testName + " is skipped on " + bwcVersion, isEnabled(testName, instructions, bwcVersion)); | ||
| assumeFalse( |
There was a problem hiding this comment.
Should the similar be removed from MultiClusterSpecIT class as well?
That looks to be safe because it's done by name. So ok! We're safe to zap this. And the comment.
Not sending that should be fine. The field will just be missing/null. That's ok. Probably should keep it in the mapping over there too to handle older cluster versions. |
This removes the undocumented `META FUNCTIONS` command that emits descriptions for all functions. This shouldn't be used because we never told anyone about it. I'd have preferred if we'd have explicitly documented it as no public or if we'd have left it snapshot-only. But sometimes you make a mistake. I'm hopeful no one is relying on it. It was never reliable and not public.....
💚 Backport successful
|
This removes the undocumented `META FUNCTIONS` command that emits descriptions for all functions. This shouldn't be used because we never told anyone about it. I'd have preferred if we'd have explicitly documented it as no public or if we'd have left it snapshot-only. But sometimes you make a mistake. I'm hopeful no one is relying on it. It was never reliable and not public.....
This removes the undocumented `META FUNCTIONS` command that emits descriptions for all functions. This shouldn't be used because we never told anyone about it. I'd have preferred if we'd have explicitly documented it as no public or if we'd have left it snapshot-only. But sometimes you make a mistake. I'm hopeful no one is relying on it. It was never reliable and not public.....
This removes the undocumented `META FUNCTIONS` command that emits descriptions for all functions. This shouldn't be used because we never told anyone about it. I'd have preferred if we'd have explicitly documented it as no public or if we'd have left it snapshot-only. But sometimes you make a mistake. I'm hopeful no one is relying on it. It was never reliable and not public.....
9 participants
This removes the undocumented
META FUNCTIONScommand that emits descriptions for all functions. This shouldn't be used because we never told anyone about it. I'd have preferred if we'd have explicitly documented it as no public or if we'd have left it snapshot-only. But sometimes you make a mistake. I'm hopeful no one is relying on it. It was never reliable and not public.....