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 389 directory server input #6117

Closed
wants to merge 9 commits into from
Closed

Add 389 directory server input #6117

wants to merge 9 commits into from

Conversation

falon
Copy link
Contributor

@falon falon commented Jul 15, 2019

Required for all PRs:

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

Hello @Dodgers, thank you for the plugin #5691! 👍
I propose to add these new features:

  • monitor each connection details if status = true
  • automonitor all dbs if alldbmonitor = true
  • added the Directory version in tags
  • unified metrics all in a row

For all contributors, tell me what do you think and feel free to modify the code.
Thank you

google2797350_student and others added 8 commits April 6, 2019 23:29
- monitor each connection details is status = true
- automonitor all dbs if alldbmonitor = true
- added the Directory version in tags
- unified metrics all in a row
Copy link

@gnulux gnulux left a comment

Choose a reason for hiding this comment

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

Many thanks for your work

@glinton
Copy link
Contributor

glinton commented Jul 22, 2019

Is the consensus that this take priority over (and we can close) #5691?

@falon
Copy link
Contributor Author

falon commented Jul 23, 2019

Is the consensus that this take priority over (and we can close) #5691?

Ok for me, thank you.
If anyone would test this plugin with Grafana, I provided some dashboards here:

@gnulux
Copy link

gnulux commented Jul 27, 2019

Ok for me too

@glinton glinton changed the title 389ds Add 389 directory server input Sep 11, 2019
@glinton glinton mentioned this pull request Sep 11, 2019
3 tasks
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

Thanks

plugins/inputs/ds389/README.md Show resolved Hide resolved
plugins/inputs/ds389/README.md Show resolved Hide resolved
plugins/inputs/ds389/README.md Outdated Show resolved Hide resolved

# reverse metric names so they sort more naturally
# Defaults to false if unset, but is set to true when generating a new config
dbtomonitor = ["db1","db2"]
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about these being filters and calling them db_include and db_exclude

filter, err := filter.NewIncludeExcludeFilter(ecs.ContainerNameInclude, ecs.ContainerNameExclude)

Copy link

Choose a reason for hiding this comment

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

I didn't implement it , cause i don't understand how it works

plugins/inputs/ds389/README.md Outdated Show resolved Hide resolved
for _, attr := range entry.Attributes {
if attr.Name == "connection" && status {
for _, thisAttr := range attr.Values {
elements := strings.Split(thisAttr, ":")
Copy link
Contributor

Choose a reason for hiding this comment

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

Verify the length of elements to avoid a potential panic when indexing the slice.

plugins/inputs/ds389/ds389.go Outdated Show resolved Hide resolved
plugins/inputs/ds389/ds389.go Outdated Show resolved Hide resolved
plugins/inputs/ds389/ds389.go Outdated Show resolved Hide resolved
plugins/inputs/ds389/ds389.go Outdated Show resolved Hide resolved
@gnulux
Copy link

gnulux commented Sep 15, 2019

I can't update this PR , do i have to create another PR ?

@falon
Copy link
Contributor Author

falon commented Sep 16, 2019

I can't update this PR , do i have to create another PR ?

Thank you for update these changes!

I don't know if it's because I opened the PR from my username...
If the cause is this, I add you to the collaborators. I don't know if this is the right way, or if there are better way to proceed...

Thank you

@glinton
Copy link
Contributor

glinton commented Sep 16, 2019

I don't know if it's because I opened the PR from my username...

That is correct. falon, you need to make the changes, or grant gnulux permissions to push to your repo.

@wisebaldone
Copy link

Hey all, is there much of glintons requests that still need to be done / anything I can help with?

@gnulux
Copy link

gnulux commented May 21, 2020

Hi all
I think, I made all the changes requested .
Fill free to update what I missed

@schlitzered
Copy link

hi, not sure if this is the right place, but it seems that this plugin will not collect cache metrics.

would you please consider to also add the metrics provided by this call:

ldapsearch -D "cn=Directory Manager" -W -p 389 -h $(hostname) -x -s base -b "cn=monitor,cn=userRoot,cn=ldbm database,cn=plugins,cn=config" "(objectclass=*)"

this for example gives you "entrycachehitratio"

@gnulux
Copy link

gnulux commented Aug 2, 2020

hi, not sure if this is the right place, but it seems that this plugin will not collect cache metrics.

would you please consider to also add the metrics provided by this call:

ldapsearch -D "cn=Directory Manager" -W -p 389 -h $(hostname) -x -s base -b "cn=monitor,cn=userRoot,cn=ldbm database,cn=plugins,cn=config" "(objectclass=*)"

this for example gives you "entrycachehitratio"

Hi,

Sorry for the late of my answer
entrycachehitratio is already available and prefixed with the backend name : userroot_entrycachehitratio .

Regards

@wisebaldone
Copy link

Hey @glinton are you happy with all the changes?

Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

I'm approving this to remove my previous block, as the majority of my previous review were addressed, though I'm no longer on the telegraf team and don't know any current patterns, etc... @reimda likely can better assist and possibly give a milestone estimate on this. Thanks for the great work!

Comment on lines 39 to 44
## Optional TLS Config
# tls_ca = "/etc/telegraf/ca.pem"
# tls_cert = "/etc/telegraf/cert.pem"
# tls_key = "/etc/telegraf/key.pem"
## Use TLS but skip chain & host verification
insecure_skip_verify = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the example config in readme.md with this as well

if err != nil {
acc.AddError(err)
return nil
}

version := sr.Entries[0].GetAttributeValue("version")
//version := sr.Entries[0].GetAttributeValue("version")
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented code

@kayssun
Copy link

kayssun commented Dec 14, 2020

Hi
Is there any update on this? This plugin would be nice to have for me.

@gnulux
Copy link

gnulux commented Dec 14, 2020 via email

@wisebaldone
Copy link

We could always makes this an execd based plugin instead. This would allow us to bump the go ldap plugin to v3 and add support for ldapi.

Ref: https://github.com/influxdata/telegraf/blob/master/plugins/common/shim/README.md

@wisebaldone
Copy link

wisebaldone commented Jan 18, 2021

Just gonna add that I would be willing to do the work for the execd based version if @falon is okay with it ( forking his original changes and building it into the shim).

@falon
Copy link
Contributor Author

falon commented Jan 18, 2021

Hello, yes, it's ok for me.
Thank you

@sjwang90 sjwang90 added the plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins label Jan 22, 2021
@sspaink
Copy link
Contributor

sspaink commented May 13, 2021

@falon thank you for your work here, how is this plugin working as an external plugin does it satisfy your needs? Would you be ok with closing this pull request?

@falon
Copy link
Contributor Author

falon commented May 14, 2021

@falon thank you for your work here, how is this plugin working as an external plugin does it satisfy your needs? Would you be ok with closing this pull request?

Hello @sspaink , maybe do you still like to add this as internal input plugin?

Yes, it works for me as external plugin. @wisebaldone create a first release for shim too, I think it works in this way.

For me is ok to use this plugin as external, anyway. @gnulux contributes a very great part on this work, so I ask his opinion.

@gnulux
Copy link

gnulux commented May 15, 2021 via email

@powersj
Copy link
Contributor

powersj commented Jan 24, 2022

Closing due to the use of an external plugin in this case.

Thanks!

@powersj powersj closed this Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants