-
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
convert airbyte-metrics-reporter to micronaut #17365
Conversation
jackson-converter-enabled: true | ||
sql-dialect: POSTGRES | ||
|
||
docker: |
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.
Is this needed for this application?
access: | ||
- isAnonymous() | ||
server: | ||
port: 9000 |
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.
9000 is the port that airbyte-workers
used before the conversion. I'm not sure if this will be an issue when running via Docker Compose, as that port is already mapped to something.
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.
@jdpgrailsdev looks like we're using the same port in a few places:
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.
It was originally added to airbyte-workers
, as it was already using port 9000. I believe it is copy/pasta in airbyte-cron
. This is probably fine because the containers/pods will handle mapping to it and we can cycle back later if we want to have the services on different ports for identification, etc.
testAnnotationProcessor libs.bundles.micronaut.test.annotation.processor | ||
|
||
// integrationTestJavaAnnotationProcessor platform(libs.micronaut.bom) | ||
// integrationTestJavaAnnotationProcessor libs.bundles.micronaut.test.annotation.processor |
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.
Nit: remove the commented out dependencies if not needed
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.
with a few minor nits/comments.
@colesnodgrass Bonus points if you want to also include a startup banner: https://github.com/airbytehq/airbyte/blob/master/airbyte-workers/src/main/resources/micronaut-banner.txt |
* @return Duration of how often this metric should report. | ||
*/ | ||
public Duration getDuration() { | ||
return Duration.ofSeconds(15); |
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 do you we need that? Isn't the Datadog agent supposed to do that for us? Won't this be an issue if we have metrics that are different than a gauge?
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.
As these are stats which are emitted to the datadog statsd agent, there is no way (that I'm currently aware of) to tell this agent to fetch these values on a period basis. They are not polled by the agent, but rather are pushed to the agent. This is why these are registered to run in the EventListeners
class at a fixed rate. The rate of which is determined by what duration is returned from this getDuration
method.
Not every value is currently running every 15 seconds, some run every hour.
As for the non-gauge values, all of these emitters should match the existing behavior from the now defunct ToEmit
enum. If they were working before they should still be working.
@@ -1 +1,8 @@ | |||
: airbyte-metrics-reporter : | |||
|
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 strong opinon here but seeing the Airbyte banner, was the sign for me that my local deployment can be use. Having many of them will be confusing. If we decide on keeping them we should add it to the cron app as well.
} | ||
|
||
List<Long> numberOfActiveConnPerWorkspace() { | ||
final var query = """ |
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.
Is there any reason to use not use JOOQ here?
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.
This is copied verbatim from the existing implementation. I didn't want to move and change them within the same PR as I find that makes it's difficult to actually track the query changes. I plan to do a follow-up PR that cleans these queries up a bit.
return ctx.fetch(query).getValues("num_conn", long.class); | ||
} | ||
|
||
long numScheduledActiveConnectionsInLastDay() { |
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.
Aren't we going to miss some connection here? expecially the cron and the manual ones that have been triggered.
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.
Possibly? This was also copied verbatim from the existing implementation. See my comments above about a follow-up PR.
and j.created_at > now() - interval '24 hours 1 minutes' | ||
and c.status = 'active' | ||
and j.config_type = 'sync' | ||
and c.updated_at < now() - interval '24 hours 1 minutes' |
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 do we need the filter on the updated_at of the connection table?
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.
This too was copied verbatim from the existing implementation. See my comments above about a follow-up PR.
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.
Make sense about the files that got moved.
This reverts commit d30de1b.
* sealed class experiment * micronaut the metric reporter service * code format * move event-listeners to EventListeners class * code format * db cleanup; start adding tests * finish porting over MetricsQueriesTest pieces that apply * dedupe/simplify test code * add EmitterTest; misc code clean-up * address pmd warnings * remove code that was moved to the reporter package * update application.yml * Emitter[] -> List<Emitter> * remove unused singleton * formatting * formatting * remove default test creds * update banner * remove commented out code * remove another commented out line
* Revert "improve query performance (airbytehq#17862)" This reverts commit e0db09b. * Revert "fix metric reporter not producing metrics (airbytehq#17863)" This reverts commit 63e39e6. * Revert "convert airbyte-metrics-reporter to micronaut (airbytehq#17365)" This reverts commit d30de1b.
What
airbyte-metrics-reporter
to be Micronaut based.How
ReporterApp.configDatabase
andMietricClientFactory.getMetricClient()
calls with constructor injected beansMetricRepository
andMetricClient
dependencies which are defined in theReporterFactory
class.metrics-lib/MetricQueries
to theMetricRepository
class in the reporter module.public
topackage-private
as there is no reason for these to be available outside of this use-case.main
function to a Micronaut annotated@EventListener
in theEventListeners
class.ReporterApp
.ApplicationStartupEvent
andApplicationShutdownEvent
events.ToEmit
enum with a sealedEmitter
class.Emitter
class is responsible for a single metric.Emitter
class has its dependencies injected via it's constructor.Emitter
class is annotated with@Singleton
and supports being injected.Emitter
class.Recommended reading order
Application.java
ReporterFactory.java
EventListeners.java
MetricRepository.java
MetricQueries.java
and slightly cleaned up, including the tests.Emitter.java
build.gradle
application.yml
Note
I will be updating some of the queries used to gather these metrics in a follow-up PR as they are inefficient. This was not done as part of this PR as it's worth reviewing those changes independent of the Micronaut changes.