feat(inputs.aliyuncms): Refactor common parts to plugins/common#18280
feat(inputs.aliyuncms): Refactor common parts to plugins/common#18280ZPascal wants to merge 2 commits intoinfluxdata:masterfrom
Conversation
telegraf-tiger
bot
added
the
feat
|
FYI: @srebhan This PR integrates the Aliyun common library. |
srebhan
changed the title
telegraf-tiger
bot
added
the
plugin/input
| var ( | ||
| roleSessionExpiration = 3600 | ||
| sessionExpiration = 3600 | ||
| ) |
There was a problem hiding this comment.
Please use
| var ( | |
| roleSessionExpiration = 3600 | |
| sessionExpiration = 3600 | |
| ) | |
| roleSessionExpiration := 3600 | |
| sessionExpiration := 3600 |
| if err != nil { | ||
| require.Contains(t, err.Error(), "failed to retrieve credential") | ||
| } else { | ||
| require.NotNil(t, cred) | ||
| } |
There was a problem hiding this comment.
No! You should know what you expect from that call! Either this should return an error, then check for it, or it doesn't!
| var ( | ||
| err error | ||
| data map[string]interface{} | ||
| lastData map[string]interface{} | ||
| ) |
There was a problem hiding this comment.
None of these is required to be declared outside of the go-routine, is it? Please use implicit declarations where possible and limit the scope of variables to a minimum to avoid unintended reuse!
| case <-dt.done: | ||
| return |
There was a problem hiding this comment.
Wouldn't using a cancel-able context be better here?
| req = "string" | ||
| require.NotNil(t, req) | ||
|
|
||
| req = 123 | ||
| require.NotNil(t, req) |
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| rpcReq, err := getRPCReqFromDiscoveryRequest(tt.req) | ||
| if tt.expectError { | ||
| require.Error(t, err) | ||
| require.Nil(t, rpcReq) | ||
| } else { | ||
| require.NoError(t, err) | ||
| require.NotNil(t, rpcReq) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Please split into a test that should succeed and one that should fail to simplify code and to provide context to the reader.
Same for other cases where you have if expectedError else constructs in tests!
| var req DiscoveryRequest | ||
|
|
||
| req = &mockDiscoveryRequest{RpcRequest: &requests.RpcRequest{}} |
There was a problem hiding this comment.
Please use implicit declarations where possible
| var req DiscoveryRequest | |
| req = &mockDiscoveryRequest{RpcRequest: &requests.RpcRequest{}} | |
| req := &mockDiscoveryRequest{RpcRequest: &requests.RpcRequest{}} |
There was a problem hiding this comment.
Is there a way to get the regions from the SDK? I don't want to maintain that list in telegraf as it is likely to be changed without anyone from Aliyun notifying us...
There was a problem hiding this comment.
I don't think this test does anything useful...
ZPascal
force-pushed
the
add-the-aliyun-common-lib
branch
2 times, most recently
from
c390979 to
37fba13
Compare
|
This PR is still missing the part where you convert |
ZPascal
force-pushed
the
add-the-aliyun-common-lib
branch
from
37fba13 to
56d4c35
Compare
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
srebhan
left a comment
srebhan
left a comment
There was a problem hiding this comment.
Thanks for the update @ZPascal! I have some more comments in the code. The biggest hurdles I see are
- The PR contains RDS code. This is unrelated to this PR and should be removed.
- The PR changes functionality in the CMS plugin. Please remove those changes and try to only move code around.
Please reduce this PR to only move code to common! Do not add functionality or change the way things are done in this PR! Submit follow-up PRs if you need to add functionality or if you need to change type-assertions, timestamp-parsing etc.
| ## - acs_rds_dashboard | ||
| ## - acs_slb_dashboard | ||
| ## - acs_vpc_eip | ||
| ## Note: RDS Performance metrics are now supported via the dedicated aliyunrds plugin |
| ## RDS Performance metrics | ||
|
|
||
| You can use the dedicated [aliyunrds](../aliyunrds/README.md) plugin to | ||
| collect RDS Performance metrics. | ||
|
|
| ## - acs_rds_dashboard | ||
| ## - acs_slb_dashboard | ||
| ## - acs_vpc_eip | ||
| ## Note: RDS Performance metrics are now supported via the dedicated aliyunrds plugin |
| } | ||
| } | ||
|
|
||
| func TestPluginMetricsRDSServiceInitialize(t *testing.T) { |
| } | ||
|
|
||
| func TestGather(t *testing.T) { | ||
| func TestRDSServiceInitialization(t *testing.T) { |
There was a problem hiding this comment.
This does not belong here! Same for all other RDS related things below!
| Log telegraf.Logger `toml:"-"` | ||
|
|
||
| client aliyuncmsClient | ||
| cmsClient aliyuncmsClient |
There was a problem hiding this comment.
Please keep the previous name to reduce the diff-size in the PR!
| } | ||
|
|
||
| // parseTimestamp normalizes various timestamp representations into seconds since epoch. | ||
| func (s *AliyunCMS) parseTimestamp(v interface{}) (int64, bool) { //nolint:revive // Valid case |
There was a problem hiding this comment.
Why do you need the nolint statement here?
| if val, err := t.Int64(); err == nil { | ||
| return val, true | ||
| } | ||
| default: |
There was a problem hiding this comment.
Please remove the default case as it doesn't do anything...
| parsed, ok := s.parseTimestamp(value) | ||
| if !ok { | ||
| s.Log.Warnf("Unexpected timestamp type %T, skipping datapoint", value) | ||
| continue NextDataPoint | ||
| } | ||
| ts = parsed |
There was a problem hiding this comment.
This is a functional extension! Please move this to an own PR!
| tags[key] = value.(string) | ||
| if metric.discoveryTags != nil { // discovery can be not activated | ||
| // Skipping data point if discovery data not exist | ||
| _, ok := metric.discoveryTags[value.(string)] | ||
| if !ok && | ||
| !metric.AllowDataPointWODiscoveryData { | ||
| s.Log.Warnf("Instance %q is not found in discovery, skipping monitoring datapoint...", value.(string)) | ||
| continue NextDataPoint | ||
| } | ||
|
|
||
| for k, v := range metric.discoveryTags[value.(string)] { | ||
| tags[k] = v | ||
| } | ||
| strVal, ok := value.(string) | ||
| if !ok { | ||
| s.Log.Warnf("Unexpected non-string %q value in datapoint, skipping...", key) | ||
| continue NextDataPoint | ||
| } | ||
| tags[key] = strVal | ||
| if keep := s.enrichTagsWithDiscovery(tags, m, strVal); !keep { | ||
| continue NextDataPoint |
|
Hello! I am closing this issue due to inactivity. I hope you were able to resolve your problem, if not please try posting this question in our Community Slack or Community Forums or provide additional details in this issue and reqeust that it be re-opened. Thank you! |
2 participants
Description
This PR adds the Aliyun common library to Telegraf.
Checklist
Related issues
related to #18253