-
Notifications
You must be signed in to change notification settings - Fork 150
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I fixed it. Rebased the wrong email out of the commits. |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
@googlebot I fixed it. should be fixed properly now |
@robpickerill Thanks a lot for this contribution. I'll try to review it soon -- I am out of office half of this week, so it may get pushed to next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @robpickerill,
Thanks once again for this. Sorry for the delay in reviewing. I've added some comments. Please take a look.
Hey Manu
No problems at all. I appreciate the feedback here. Let me work through the comments, and let you know when they are complete. Thanks again. |
Manu, I've been resolving the comments so I can see which comments are still outstanding - if I've missed anything please feel free to unresolve. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the comments. I've added a few more comments, but you don't need to fix them. I am working on importing/exporting this change following the process the described at:
https://github.com/google/cloudprober/blob/master/CONTRIBUTING.md#source-of-truth-sot-and-commit-process
I'll make minor fixes (mentioned in the comments) while doing that.
|
||
switch lu { | ||
case time.Second: | ||
metricDatum.Value = aws.Float64(value * 1000) | ||
case time.Microsecond: | ||
metricDatum.Value = aws.Float64(value / 1000) | ||
case time.Nanosecond: | ||
metricDatum.Value = aws.Float64((value / 1000) / 1000) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do this instead:
metricDatum.Value = aws.Float64(value * float64(latencyUnit) / float64(time.Millisecond))
|
||
// publish the metrics to cloudwatch, using the namespace provided from configuration | ||
func (cw *CWSurfacer) publishMetrics(md *cloudwatch.MetricDatum) { | ||
if len(cw.cwMetricDatumCache) >= 20 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to use cloudwatchMaxMetricDatums here instead of 20.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, that's an obvious miss by me - but thanks for catching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. :)
return failure, nil | ||
} | ||
|
||
func (s *CWSurfacer) ignoreMetric(name string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Receiver "s" needs to consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, sorry, thats as obvious miss by me, but thanks again for catching - was copy/paste as I was trying to keep consistent with the stackdriver implementation. Apologies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries.
This is import of the original PR at #583. PiperOrigin-RevId: 369784229
@robpickerill Importing this PR through #587. Documentation changes are not pushed through the import-export method. They can be committed directly. Can you please send another PR, containing just the documentation changes? You can close this PR afterwards. |
Thanks for all your assistance here Manu, very much appreciated.
Let me wrap this up today and we can close down this PR, let me also put the items we discussed on regex's into issues for tracking, then this PR is good to close, and I'll work on datadog. Thanks again! |
This is import of the original PR at #583. PiperOrigin-RevId: 369784229
closing for #587 |
This PR adds cloudwatch surfacer support for cloudprober.
This is deployed onto AWS ECS and working with the following config:
Using this IAM policy attached to the task role: