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 fields to info metricset of Redis module #7695

Merged
merged 20 commits into from Jul 30, 2018

Conversation

Projects
None yet
4 participants
@a3dho3yn
Contributor

a3dho3yn commented Jul 24, 2018

Memory fragmentation ratio, active defragmentation stats, slave status and some other fields was missing from info metricset.

@elasticmachine

This comment has been minimized.

Show comment
Hide comment
@elasticmachine

elasticmachine Jul 24, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine commented Jul 24, 2018

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ruflin ruflin requested a review from jsoriano Jul 24, 2018

@a3dho3yn a3dho3yn changed the title from Add fields to `info` metricset of Redis module to Add fields to info metricset of Redis module Jul 24, 2018

@jsoriano

Thanks for these improvements! 🙂

- name: used.rss
type: long
format: bytes
description: >
Used memory rss.
Number of bytes that Redis allocated as seen by the operating system (a.k.a resident set size).

This comment has been minimized.

@jsoriano

jsoriano Jul 25, 2018

Member

Thanks for these improvements in the descriptions! 🙂

@jsoriano

jsoriano Jul 25, 2018

Member

Thanks for these improvements in the descriptions! 🙂

"priority": c.Int("slave_priority", s.Optional),
"is_readonly": c.Bool("slave_read_only", s.Optional),
},
// ToDo find a way to add dynamic object of slaves: "slaves": s.Str("slaveXXX")

This comment has been minimized.

@jsoriano

jsoriano Jul 25, 2018

Member

Maybe slaves data could be in a specific metricset.

But something interesting we could add here are some aggregated metrics obtained from this list of slaves, like number of slaves in each state, average/min/max lag, min/max offset...

This can be left for another PR in any case.

@jsoriano

jsoriano Jul 25, 2018

Member

Maybe slaves data could be in a specific metricset.

But something interesting we could add here are some aggregated metrics obtained from this list of slaves, like number of slaves in each state, average/min/max lag, min/max offset...

This can be left for another PR in any case.

This comment has been minimized.

@a3dho3yn

a3dho3yn Jul 25, 2018

Contributor

Here and in some other scenarios, I need a dynamic array in the schema. Is there any way to do so? Something like this JSON:

{
  "slaves": [
      {
          "host": "localhost:6380",
          "lag": 2
      },
      {
          "host": "localhost:6381",
          "lag": 0
      },   
  ]
}
@a3dho3yn

a3dho3yn Jul 25, 2018

Contributor

Here and in some other scenarios, I need a dynamic array in the schema. Is there any way to do so? Something like this JSON:

{
  "slaves": [
      {
          "host": "localhost:6380",
          "lag": 2
      },
      {
          "host": "localhost:6381",
          "lag": 0
      },   
  ]
}

This comment has been minimized.

@jsoriano

jsoriano Jul 25, 2018

Member

This kind of structure can be problematic metric-wise, to make it useful, slaves should be a nested object, if not the event would be indexed as something like:

{
  "slaves": {
    "host": ["localhost:6380", "localhost:6381"]
    "lag": [2, 0],
  }  
}

With this it is not possible, or very difficult, to make proper queries with filters or aggregations.

The problem with nested objects is that we are not using them in beats and they would require some additional support, as well as some performance tests. Kibana would also need better support for queries and visualizations on these kind of fields.

This is why I proposed to have an additional metricset, that could have events with fields like:

"slave": {
    "host": "localhost:6380",
    "lag": 2
}

Or to have by now aggregations directly on the existing metricset.

@jsoriano

jsoriano Jul 25, 2018

Member

This kind of structure can be problematic metric-wise, to make it useful, slaves should be a nested object, if not the event would be indexed as something like:

{
  "slaves": {
    "host": ["localhost:6380", "localhost:6381"]
    "lag": [2, 0],
  }  
}

With this it is not possible, or very difficult, to make proper queries with filters or aggregations.

The problem with nested objects is that we are not using them in beats and they would require some additional support, as well as some performance tests. Kibana would also need better support for queries and visualizations on these kind of fields.

This is why I proposed to have an additional metricset, that could have events with fields like:

"slave": {
    "host": "localhost:6380",
    "lag": 2
}

Or to have by now aggregations directly on the existing metricset.

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Jul 25, 2018

Member

jenkins, test this

Member

jsoriano commented Jul 25, 2018

jenkins, test this

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Jul 25, 2018

Member

@a3dho3yn you will need to update the branch and resolve the conflicts.

Member

jsoriano commented Jul 25, 2018

@a3dho3yn you will need to update the branch and resolve the conflicts.

a3dho3yn added some commits Jul 25, 2018

},
},
"slowlog": s.Object{
"count": c.Int("slowlog_len"),

This comment has been minimized.

@jsoriano

jsoriano Jul 26, 2018

Member

Don't forget to add this to the fields.yml file 🙂

@jsoriano

jsoriano Jul 26, 2018

Member

Don't forget to add this to the fields.yml file 🙂

a3dho3yn added some commits Jul 27, 2018

@a3dho3yn

This comment has been minimized.

Show comment
Hide comment
@a3dho3yn

a3dho3yn Jul 27, 2018

Contributor

I don't know why test_template (from test_base.py) fails :(

Contributor

a3dho3yn commented Jul 27, 2018

I don't know why test_template (from test_base.py) fails :(

a3dho3yn added some commits Jul 30, 2018

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Jul 30, 2018

Member

@a3dho3yn run make update after modifying the fields

Member

jsoriano commented Jul 30, 2018

@a3dho3yn run make update after modifying the fields

@a3dho3yn

This comment has been minimized.

Show comment
Hide comment
@a3dho3yn

a3dho3yn Jul 30, 2018

Contributor

I've run make update but surprisingly it did not updated docs :|

Contributor

a3dho3yn commented Jul 30, 2018

I've run make update but surprisingly it did not updated docs :|

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Jul 30, 2018

Member

Oh yes, sorry, you also need to update your branch with master.

Member

jsoriano commented Jul 30, 2018

Oh yes, sorry, you also need to update your branch with master.

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Jul 30, 2018

Member

jenkins, test this please

Member

jsoriano commented Jul 30, 2018

jenkins, test this please

@jsoriano jsoriano merged commit ce9048c into elastic:master Jul 30, 2018

6 checks passed

CLA Commit author has signed the CLA
Details
Hound No violations found. Woof!
beats-ci Build finished.
Details
codecov/patch 54.54% of diff hit (within 100% threshold of 64.71%)
Details
codecov/project 64.76% (+0.05%) compared to f771f2d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Jul 30, 2018

Member

@a3dho3yn merged, thanks a lot for this contribution and the others you are working on 😃

Just curious, are you using the provided dashboards for redis and rabbitmq? We'd be happy to accept contributions also there if you have any proposal 🙂

Member

jsoriano commented Jul 30, 2018

@a3dho3yn merged, thanks a lot for this contribution and the others you are working on 😃

Just curious, are you using the provided dashboards for redis and rabbitmq? We'd be happy to accept contributions also there if you have any proposal 🙂

@a3dho3yn

This comment has been minimized.

Show comment
Hide comment
@a3dho3yn

a3dho3yn Jul 30, 2018

Contributor

Thank you very much for all the help to complete these PRs.
We just started using ELK stack for monitoring our applications and databases. I didn't work with dashboards yet, but I think I will start working with them soon and then I will contribute if I find some deficiencies or issues :)

Contributor

a3dho3yn commented Jul 30, 2018

Thank you very much for all the help to complete these PRs.
We just started using ELK stack for monitoring our applications and databases. I didn't work with dashboards yet, but I think I will start working with them soon and then I will contribute if I find some deficiencies or issues :)

jsoriano added a commit to jsoriano/beats that referenced this pull request Aug 20, 2018

Add fields to info metricset of Redis module (elastic#7695)
Add memory fragmentation ratio, active defragmentation stats, slave status and some other fields that were missing from info metricset.

(cherry picked from commit ce9048c)

@jsoriano jsoriano added the v6.5.0 label Aug 20, 2018

jsoriano added a commit to jsoriano/beats that referenced this pull request Aug 20, 2018

Add fields to info metricset of Redis module (elastic#7695)
Add memory fragmentation ratio, active defragmentation stats, slave status and some other fields that were missing from info metricset.

(cherry picked from commit ce9048c)

ruflin added a commit that referenced this pull request Aug 23, 2018

Add fields to info metricset of Redis module (#7695) (#8021)
Add memory fragmentation ratio, active defragmentation stats, slave status and some other fields that were missing from info metricset.

(cherry picked from commit ce9048c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment