-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Move to DogStatsD. #10238
Move to DogStatsD. #10238
Conversation
airbyte-metrics/src/main/java/io/airbyte/metrics/DogstatsdMetricSingleton.java
Outdated
Show resolved
Hide resolved
airbyte-metrics/src/main/java/io/airbyte/metrics/DogstatsdMetricSingleton.java
Outdated
Show resolved
Hide resolved
return instance; | ||
} | ||
|
||
public synchronized static void initialize(final String appName, final Map<String, String> mdc, final boolean publish) { |
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.
Why is the MDC important here? Shouldn't the initialization be run from some thread that has the MDC configured?
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.
good point - I think it's left over from when I was testing. You should be right that we don't need this anymore. Will remove.
|
||
public static synchronized DogstatsdMetricSingleton getInstance() { | ||
if (instance == null) { | ||
throw new RuntimeException("You must initialize configuration with the initializeMonitoringServiceDaemon() method before getting an instance."); |
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.
Should this say .. with the initialize() method
?
Also, what is the downside of combining getInstance()
and initialize()
into the same method? The behavior would be that if instance == null
, it initializes and returns. If instance != null
, it just returns. That approach feels like it would be a bit more ergonomic, no?
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.
ah yes good catch on the error message.
this is a small thing - the main downside would be the initialise
method has more parameters than the getInstance
method. we'd have to combine those parameters if we combined the methods. because of this I prefer to keep these separate for now.
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.
@davinchia I'm curious how you are thinking about consumers of this class. If I want to use the DogstatsdMetricSingleton, how do I know if it has been initialized or not? Is it expected that a consumer would call getInstance
wrapped with a try-catch to catch a RuntimeException in the case that it was already initialized, and call initialize
in that case? That doesn't feel very ergonomic to me, but let me know if I'm thinking about this wrong
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.
Good question - I'm trying to follow the general singleton pattern where initialize
is called in the main method, and getInstance
is called everywhere else. Although it's possible to catch, most folks don't really catch the RTE since it's usually understood the singleton is initialised when the JVM first starts up.
I agree with you having to do getInstance
is somewhat annoying, but this is a fairly common pattern and I don't really think it's worth trying to get rid of.
I can't see how to not have a simply method signature if we combine initialize
and getInstance
and I didn't want to spend more time playing with this.
Happy to discuss more/review any ideas/approaches you come up with!
airbyte-scheduler/app/src/main/java/io/airbyte/scheduler/app/SchedulerApp.java
Show resolved
Hide resolved
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.
Left a couple of fairly small comments but looks good to me overall
What
We want to move to DogStatsD to make it easier to do more complex metrics collection. The official DD library also does not double count metrics so using this is cheaper overall.
Unfortunately unlike Prometheus, there is no good way to test that metrics are being reported in an automated test so I've omitted that for now. I've tested this manually in the dev env to make sure metrics are being emitted.
How
I will remove the old Prometheus class in a follow up PR.
Recommended reading order
DogStatsdMetricSingleton.java
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes