-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat: otel collector integration #4769
Conversation
24dd4ca
to
9f26eac
Compare
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 the formatter might have been misconfigured. There are many styling changes which seem to be unrelated and make it hard to review
9f26eac
to
894d1d4
Compare
acc6b0c
to
875467b
Compare
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! Incredibly clean and easy to read code! I've posted some comments and am wondering about the lack of tests and docs, but other than that it looks sweet. Sorry for the long wait for the review, I just forgot about the pr.
export const provider = gardenPlugin.createProvider({ configSchema: providerConfigSchema, outputsSchema: s.object({}) }) | ||
|
||
provider.addHandler("getEnvironmentStatus", async ({ ctx }) => { | ||
return { ready: false, outputs: {} } |
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 the hardcoded ready: false
intentional?
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 believe it is. If I recall this correctly, garden would check first if the environment is ready, and if so it would not call prepareEnvironment
again.
We need to call prepareEnvironment
every time, so we always say it's not ready.
Even worse is that if you just once leave out this hook, the status is assumed to be complete with cache enabled, and the cached state is written to the filesystem making it that without a --force
the provider will never initialize again.
We also found that returning disableCache: true
in getEnvironmentStatus
does not actually disable the cache and will still proceed caching the state.
Thus it is required to return ready: false
here and then later on return disableCache: true
from the prepareEnvironment
handler.
|
||
exporters: | ||
- name: | ||
|
||
enabled: | ||
|
||
verbosity: normal |
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.
Will the docs come later?
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.
The problem here is that oneOf
and anyOf
schemas aren't correctly converted into docs yet.
I think we should fix this soon since it makes the docs incomplete, but since it's not strictly related and once fixed the docs will show up for this as well, I decided to not make it part of this PR.
@shumailxyz @Orzelius I thought about how to test this but couldn't come up with a good way. |
I think a e2e test is the way to go here. It would serve as a smoke test for if we make a fatal mistake that would kill the functionality completely and would double as an example configuration for when we need to do further work on the functionality. We already have some clumsy e2e tests that do a lot, but that's the business we're in |
@@ -90,13 +90,18 @@ export async function shutdown(code?: number) { | |||
// eslint-disable-next-line no-console | |||
console.log(getDefaultProfiler().report()) | |||
} | |||
process.exit(code) | |||
gracefulExit(code) |
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.
🚀 Nice that's much better
90bd1f7
to
225666f
Compare
225666f
to
72fedd4
Compare
@shumailxyz @Orzelius Added some e2e smoke tests for the feature. |
…treamLogs` do it implicitly
744bc03
to
e90d92d
Compare
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.
Looks good, but what about docs 👉👈?
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.
@TimBeyer and I decided that the provider reference docs should suffice. This is good to merge from my side. cc @garden-io/core
What this PR does / why we need it:
This PR enables us to locally deploy an otel collector by configuring it as a provider.
The provider once configured finds an open port, creates a configuration file on the fly and then spawns the collector on the local machine. It then reconfigures the existing otel exporter to target the new collector, which then proceeds to export the data to the desired destination.
The collector gracefully shuts down at the end of the garden session after attempting to flush all remaining data.
An example project config could look like this:
Currently supported are a generic
otlphttp
type for any HTTP OTLP endpoint, and configs fornewrelic
,honeycomb
anddatadog
.The environment variable
GARDEN_ENABLE_TRACING
is nowtrue
by default, allowing just the override to disable it.When not configured however, it sets up a "no op" exporter so that we're not leaking memory.
By default no data is sent anywhere without explicit configuration, this is just so that there's still an override for disabling it but there's no need to manually set it to enable the collector provider to work.
If a user would like to override the exporter at the lowest level, skipping the provider based setup and just using an environment variable, that is still possible via setting
OTEL_TRACES_EXPORTER
based on the options documented in https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-sdk-node/README.md#configure-trace-exporter-from-environmentFor example to trace to a JSON HTTP OTLP endpoint on localhost, one could use
In the majority of cases that should not be necessary, but it still does exist in case a user has their own OTEL setup and does not want to deploy an additional collector via the provider.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: