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

Draft: Cloudwatch publisher opt in #445

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

krimsz
Copy link
Contributor

@krimsz krimsz commented Jul 4, 2022

📢 Type of change

  • Bugfix
  • New feature
  • [X ] Enhancement
  • Refactoring

📜 Description

Enable CloudWatch metrics if dependency is in the path

💡 Motivation and Context

Enhancement #345

💚 How did you test it?

In progress...

📝 Checklist

In progress...

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

Still a draft, wip

@github-actions github-actions bot added the component: core Core functionality related issue label Jul 4, 2022
@krimsz krimsz force-pushed the cloudwatch-publisher-opt-in branch from cfc6064 to 88af5ac Compare July 4, 2022 07:33
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

Thanks guys for a PR! I left few comments after quick look. I'll wait with deeper review till you make it non-draft..? Or is there something specific you would like to get feedback on before?

Hint: run make format before pushing or build with make build to avoid having CrossRegionS3Client modified.

@krimsz
Copy link
Contributor Author

krimsz commented Jul 5, 2022

Hey @maciejwalkowiak its just me although it shows 2 people (changed laptops and forgot to change my git user to push a couple commits, will amend that before removing the draft status)

Still working on it, tests are failing here because no region provider (worked on my machine because I have a default REGION set locally). Will work on that, fix the things you mentioned and add the parameterstore and secretmanager because they are not following the normal autoconfiguration pattern.

The PR is a draft as I'm still workign on it, will notify when ready to review, just wanted to open it to show that it's actively worked on

@krimsz krimsz force-pushed the cloudwatch-publisher-opt-in branch from 82b2ebb to 91a64bd Compare July 5, 2022 07:48
@maciejwalkowiak
Copy link
Contributor

Thanks @krimsz! Keep it going, great work and ping me in case you have any questions.

@sonarcloud
Copy link

sonarcloud bot commented Jul 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@krimsz
Copy link
Contributor Author

krimsz commented Jul 11, 2022

@maciejwalkowiak if you can please take a look at around how I'm creating (probably in a very wrong way) at AbstractAwsConfigDataLocationResolver . ParameterStore and SecretsManager are being created differently but I seem to be unable to find the propper place and way to create my MetricsPublisher. Code looks very ugly and I would like to get some direction before I keep going this direction.

My main issue with the approach here (besides the ugliness since I don't have access to the full context with full bean creation) is how to test it properly. The way the clients hide the builder makes it impossible to access the metricsPublisher at all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: core Core functionality related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants