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

Fix for #9135 #9156

Merged
merged 9 commits into from
Apr 29, 2021
Merged

Fix for #9135 #9156

merged 9 commits into from
Apr 29, 2021

Conversation

i-prudnikov
Copy link
Contributor

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.

resolves #9135

According to symptoms, it looks like Alicloud changes the API behaviour, this fix is to address more strict metrics querying specifying region.
So we need to specify region for metric gathering. As we already have discovery region, I've decided to obsolete discovery_region attribute in favour of regions attribute that specify both: metrics and discovery scope.

Old configs are still supported and deprecation warning is printed if discovery_region is used.
In case only discovery_regions is specified then regions is populated from it. If both specified then discovery_regions is omitted.

@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Apr 20, 2021
Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@i-prudnikov
Copy link
Contributor Author

i-prudnikov commented Apr 20, 2021

!signed-cla

Copy link
Contributor

@ivorybilled ivorybilled left a comment

Choose a reason for hiding this comment

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

Provided that this solves the issue that other users were having, which it sounds like it does, this makes sense to me.

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

@vipinvkmenon
Copy link
Contributor

What a relief :D.... I had opened to create this PR myself....if there wasn't movement :)

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this so quickly! I have some comments in the code. Exporting of the discovery part and the channel seems unnecessary, so please revert if you don't have any good reason of doing it. :-)
@vipinvkmenon can you please test if this fixes your problem and add a comment!?

@srebhan srebhan self-assigned this Apr 21, 2021
@vipinvkmenon
Copy link
Contributor

Tried another configuration:

[agent]
  [[inputs.aliyuncms]]
   regions = ["eu-central-1"]
   project = "acs_nat_gateway"
   access_key_id  = "xxx"
   access_key_secret = "xxx"
   period = "10m"
   [[inputs.aliyuncms.metrics]]
     names = ["SessionNewConnection"]
   allow_dps_without_discovery = false

  [[outputs.file]]
    files = ["stdout"]

got the output:

 ./telegraf --config confx
2021-04-21T17:24:41Z I! Starting Telegraf
2021-04-21T17:24:41Z I! Loaded inputs: aliyuncms
2021-04-21T17:24:41Z I! Loaded aggregators:
2021-04-21T17:24:41Z I! Loaded processors:
2021-04-21T17:24:41Z I! Loaded outputs: file
2021-04-21T17:24:41Z I! Tags enabled: host=xxx
2021-04-21T17:24:41Z I! [agent] Config: Interval:10s, Quiet:false, Hostname:"xxx", Flush Interval:10s
2021-04-21T17:24:41Z W! [inputs.aliyuncms] 'discovery_regions' is deprecated and ignored in favour of 'regions'.
2021-04-21T17:24:41Z E! [inputs.aliyuncms] Discovery tool is not activated: no discovery support for project "acs_nat_gateway"
aliyuncms_acs_nat_gateway,host=xxx,instanceId=nxw-gwxxxxxxxx,userId=11xxx session_new_connection_value=10 1619025600000000000

Indicating the plugin works for acs_nat_gateway and also the property allow_dps_without_discovery = false works

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

It seems like the code is not yet reflecting the resolved state in the comment. Did you maybe forget to push your latest changes?

@i-prudnikov
Copy link
Contributor Author

It seems like the code is not yet reflecting the resolved state in the comment. Did you maybe forget to push your latest changes?

Sorry, looks like I forgot to push changes. Pushed just now.

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Almost there. Just a few minor things, see comments in the code...

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Code looks good. @i-prudnikov the bot thinks you are missing the CLA, if you already signed it, please write a comment with !signed-cla. Let me know if there are problems...

@i-prudnikov
Copy link
Contributor Author

Code looks good. @i-prudnikov the bot thinks you are missing the CLA, if you already signed it, please write a comment with !signed-cla. Let me know if there are problems...

Hello, I've already put the comment with !signed-cla, here it is once again: !signed-cla

@i-prudnikov
Copy link
Contributor Author

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

!signed-cla

Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

🤝 ✅ CLA has been signed. Thank you!

@sspaink
Copy link
Contributor

sspaink commented Apr 22, 2021

@i-prudnikov sorry about the CLA bot confusion, there was an issue that should be resolved now.

@i-prudnikov
Copy link
Contributor Author

@i-prudnikov sorry about the CLA bot confusion, there was an issue that should be resolved now.

Sure, NP!
@sspaink, @reimda, @srebhan, sorry for the off-topic, but could somebody review this: #7749

@reimda
Copy link
Contributor

reimda commented Apr 22, 2021

@sspaink, @reimda, @srebhan, sorry for the off-topic, but could somebody review this: #7749

Sure, we'll take a look. thanks!

@vipinvkmenon
Copy link
Contributor

@i-prudnikov in aliyuncms acs_ecs_dashboard etc are called Namespace but in the plugin, you've called it Project. Any reason for the same? Maybe some documentation to understand this.

@i-prudnikov
Copy link
Contributor Author

i-prudnikov commented Apr 23, 2021

@i-prudnikov in aliyuncms acs_ecs_dashboard etc are called Namespace but in the plugin, you've called it Project. Any reason for the same? Maybe some documentation to understand this.

@vipinvkmenon , I think that the source of confusion is this page: https://help.aliyun.com/document_detail/28619.html?spm=a2c4g.11186623.6.690.1938ad41wg8QSq - here they use term project, but in the API spec (for example here: https://help.aliyun.com/document_detail/51936.html?spm=a2c4g.11186623.2.18.2bc1750eeOw1Pv) they are using term namespace.

@srebhan
Copy link
Member

srebhan commented Apr 23, 2021

@vipinvkmenon can we please stay on-topic here! ;-P There is a slack channel to discuss this kind of things... :-)

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Apr 23, 2021
@vipinvkmenon
Copy link
Contributor

From the test perspective the PR works :)

Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

lgtm! @i-prudnikov once you've resolved the conflicts, I will merge this pr. Thanks!

@sspaink sspaink merged commit e3ae7ca into influxdata:master Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proper Configuration for inputs.aliyuncms
6 participants