Skip to content
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

Add aggregation list to node info #60074

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Jul 22, 2020

There are at least two use cases (transform tests and telemetry mapping
updated) in which we need to have an access to full list of supported
aggregations. This PR adds this list to the node info API.

Fixes #52057

There are at least two use cases (transform tests and telemetry mapping
updated) in which we need to have an access to full list of supported
aggregations. This PR adds this list to the node info API.

Fixes elastic#59774
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Features)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@elasticmachine elasticmachine added Team:Data Management Meta label for data/management team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Jul 22, 2020
Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good. a couple comments. do you have an example of the output ?

}
}

public Map<String, Set<String>> getProcessors() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getProcessors() ? I assume the name is off, but is this even needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy and paste error. Will fix.

metric: [ aggregations ]

# if this test failed because a new aggregation was added, please open an issues in the elastic/telemetry repository
# so they can update the mapping accordingly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment seems abit out of place, and probably shouldn't reference a private repo.

also, I guess I don't understand the output. is it aggregrations.??.types = ?? . Is there anything that prevents checking for known types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was sure I removed it. This comment is out of place. Good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output looks like this:

      "aggregations" : {
        "adjacency_matrix" : {
          "types" : [
            "other"
          ]
        },
        "auto_date_histogram" : {
          "types" : [
            "boolean",
            "date",
            "numeric"
          ]
        },
        "avg" : {
          "types" : [
            "boolean",
            "date",
            "histogram",
            "numeric"
          ]
        },
        "boxplot" : {
          "types" : [
            "histogram",
            "numeric"
          ]
        },
        "cardinality" : {
          "types" : [
            "boolean",
            "bytes",
            "date",
            "geopoint",
            "geoshape",
            "ip",
            "numeric",
            "range"
          ]
        },
        ....

and both types and aggs change depending on license that this is ran under.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@imotov imotov requested a review from not-napoleon July 23, 2020 17:35
Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine to merge, but if we can make AggregationInfo immutable, I think that would be better.

}

public Map<String, Set<String>> getAggregations() {
return aggs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're leaking a mutable reference here. It doesn't look like we mutate it anywhere, might be worth wrapping it to be immutable, if the object creation cost for the wrapper isn't too much.

@imotov imotov merged commit 20c5f7e into elastic:master Jul 27, 2020
imotov added a commit to imotov/elasticsearch that referenced this pull request Jul 27, 2020
Adds a full list of supported aggregations to the node info API. This list
will be used in transform tests and telemetry mapping tests that will be added
as follow-up PRs.

Fixes elastic#59774
imotov added a commit that referenced this pull request Jul 28, 2020
Adds a full list of supported aggregations to the node info API. This list
will be used in transform tests and telemetry mapping tests that will be added
as follow-up PRs.

Fixes #59774
@imotov imotov deleted the issue-52057-aggs-in-node-info branch July 28, 2020 19:33
@andreidan andreidan removed the :Analytics/Aggregations Aggregations label Oct 8, 2020
@elasticmachine elasticmachine removed the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 8, 2020
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add aggregation list to node info
5 participants