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

Kibana Input Plugin: added compatibility for all 6.X.X versions #6923

Merged
merged 5 commits into from
Jan 23, 2020
Merged

Kibana Input Plugin: added compatibility for all 6.X.X versions #6923

merged 5 commits into from
Jan 23, 2020

Conversation

vikkyomkar
Copy link
Contributor

@vikkyomkar vikkyomkar commented Jan 18, 2020

closes #6894
closes #5744

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@vikkyomkar
Copy link
Contributor Author

this PR fixes #6894 and #5744

@vikkyomkar
Copy link
Contributor Author

@danielnelson can we get some eyes on this PR ?

@lpic10
Copy link
Contributor

lpic10 commented Jan 21, 2020

With these changes the plugin seems to work up to version 7.5 (there are no changes to these metrics even in master branch: https://github.com/elastic/kibana/blob/master/src/legacy/server/status/lib/metrics.js)

@vikkyomkar The change on kibana that needs the fix happened on version 6.4, not 6.5, so it would be better to change the PR to reflect that.

Also, there is a metric name change, but the underlying metric seems to be the same:
https://github.com/elastic/kibana/blob/cb16be83a47c5bc8a5214c2461dddb8c2299f6f5/src/server/status/metrics.js#L32
https://github.com/elastic/kibana/blob/564b25f2ab7b33545e38980443c24162f6ad6d4e/src/server/status/lib/metrics.js#L74

I don't know if it is a good practice to change the metric name on telegraf. OTOH I think the metric name in the updated kibana makes more sense.

@danielnelson what do you recommend?

Copy link
Contributor

@danielnelson danielnelson 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 we should either record this only as the older name heap_max_bytes or as both heap_total_bytes and heap_max_bytes.

fields["response_time_avg_ms"] = kibanaStatus.Metrics.ResponseTimes.AvgInMillis
fields["response_time_max_ms"] = kibanaStatus.Metrics.ResponseTimes.MaxInMillis
fields["requests_per_sec"] = float64(kibanaStatus.Metrics.Requests.Total) / float64(kibanaStatus.Metrics.CollectionIntervalInMilles) * 1000

Version, err := strconv.ParseFloat(strings.Join(strings.Split(kibanaStatus.Version.Number, ".")[:2], "."), 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line could panic with unexpected version info, in particular when indexing [:2]. Check the length of the split item to ensure it is long enough. To match Go naming convention use a lowercase variable version :=.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielnelson code is updated to handle any panic situation


if Version >= 6.5 {
fields["uptime_ms"] = kibanaStatus.Metrics.Process.UptimeInMillis
fields["heap_total_bytes"] = kibanaStatus.Metrics.Process.Memory.Heap.TotalInBytes
Copy link
Contributor

Choose a reason for hiding this comment

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

For backwards compatibility, either record this only as only heap_max_bytes or as both heap_total_bytes and heap_max_bytes. Make sure to add a comment so we can remember why we did this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

code updated along with the comments as per the suggestions

Comment on lines 61 to 67
```
Kibana Version < 6.5
kibana,host=myhost,name=my-kibana,source=localhost:5601,status=green,version=6.3.2 concurrent_connections=0i,heap_max_bytes=136478720i,heap_used_bytes=119231088i,requests_per_sec=1,response_time_avg_ms=59i,response_time_max_ms=300i,status_code=1i,uptime_ms=2187428019i 1534864502000000000

kibana,host=myhost,name=my-kibana,source=localhost:5601,version=6.3.2 concurrent_connections=0i,heap_max_bytes=136478720i,heap_used_bytes=119231088i,response_time_avg_ms=0i,response_time_max_ms=0i,status="green",status_code=1i,uptime_ms=2187428019i 1534864502000000000
Kibana Version > 6.5
kibana,host=myhost,name=my-kibana,source=localhost:5601,status=green,version=6.5.4 concurrent_connections=8i,heap_total_bytes=447778816i,heap_used_bytes=380603352i,requests_per_sec=1,response_time_avg_ms=57.6,response_time_max_ms=220i,status_code=1i,uptime_ms=6717489805i 1534864502000000000
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Only show the >6.5 version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plugin output is made generic for all the versions

@danielnelson danielnelson added fix pr to fix corresponding bug feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Jan 22, 2020
@danielnelson danielnelson added this to the 1.13.3 milestone Jan 23, 2020
@danielnelson danielnelson merged commit bc3429e into influxdata:master Jan 23, 2020
danielnelson pushed a commit that referenced this pull request Jan 23, 2020
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin fix pr to fix corresponding bug
Projects
None yet
3 participants