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

Convert license struct to MapStr #14378

Merged
merged 4 commits into from
Nov 12, 2019

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Nov 1, 2019

Currently when the elasticsearch module prepares a cluster_stats document to index into .monitoring-es-*, it marshals any values representing ms-since-epoch into scientific notation, e.g. 1.572634502804E12. Elasticsearch needs these values to be preserved as regular integers, e.g. 1572634502804.

Turns out this only happens when marshaling a struct containing such values. Marshaling a common.MapStr marshals the values as expected.

Looking through the elasticsearch module code for the cluster_stats metricset, the only struct that was being used in preparing the document for ES was the License struct. This PR adds a toMapStr() method to this struct and uses its results in the document instead.

Testing this PR

  1. Start up Elasticsearch.

  2. Build Metricbeat with this PR.

    cd $GOPATH/src/github.com/elastic/beats/metricbeat
    mage build
    
  3. Enable the elasticsearch-xpack Metricbeat module.

 ./metricbeat modules enable elasticsearch-xpack
  1. Run Metricbeat

    ./metricbeat -e
    
  2. Verify that type:cluster_stats documents in .monitoring-es-* contain regular integer values for ms-since-epoch fields under the license field.

    GET .monitoring-es-*/_search?q=type:cluster_stats&sort=timestamp:desc&filter_path=hits.hits._source.license*
    

@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring (Stack monitoring)

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I tried to test it but I think it requires the platinum ES license. Any unit test you can add for making sure ms-since-epoch values are marshaled correctly?

@jakelandis
Copy link

Changes looks good (but I am not terribly familiar with this code base or language). Thanks for fixing this !

@ycombinator
Copy link
Contributor Author

The code looks good to me. I tried to test it but I think it requires the platinum ES license.

Trial license works as well.

Any unit test you can add for making sure ms-since-epoch values are marshaled correctly?

I've been trying to do this for the past couple hours, actually. From what I can tell the marshalling from int to double is happening at a lower level, perhaps in libbeat. So I'm not sure a simple unit test in the elasticsearch Metricbeat module would do the trick here. Since the symptom of the problem shows up in an Elasticsearch document, I might have to resort to a system test here.

@ycombinator
Copy link
Contributor Author

ycombinator commented Nov 2, 2019

@kaiyan-sheng I've added a system test per my previous comment. Please re-review when you get a chance. Thanks!

[EDIT] FWIW, I ran the same system test on master and it fails (as expected)!

Copy link
Contributor

@kaiyan-sheng kaiyan-sheng left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the test.

@ycombinator ycombinator force-pushed the mb-es-license-ts branch 2 times, most recently from 2e3b0d6 to 9d53df9 Compare November 7, 2019 14:11
Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

Tested and works as expected. Code looks good as well. Nice work!

@ycombinator ycombinator merged commit 9a48b04 into elastic:master Nov 12, 2019
@ycombinator ycombinator deleted the mb-es-license-ts branch November 12, 2019 01:02
ycombinator added a commit that referenced this pull request Nov 13, 2019
* Convert license struct to MapStr

* Adding CHANGELOG entry

* Fixing structure

* Adding system test
ycombinator added a commit that referenced this pull request Nov 13, 2019
* Convert license struct to MapStr

* Adding CHANGELOG entry

* Fixing structure

* Adding system test
ycombinator added a commit that referenced this pull request Nov 15, 2019
* Convert license struct to MapStr (#14378)

* Convert license struct to MapStr

* Adding CHANGELOG entry

* Fixing structure

* Adding system test

* Fixing CHANGELOG due to bad rebase

* Fixing CHANGELOG
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
* Convert license struct to MapStr

* Adding CHANGELOG entry

* Fixing structure

* Adding system test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants