feat: add settings for OpenTelemetry export #2922
Conversation
| @@ -118,6 +124,9 @@ export const EnvironmentSettings: React.FC = () => { | |||
| setSlackConnectedChannel(slack_notifications_channel); | |||
|
|
|||
| setEnvVariables(env_variables); | |||
|
|
|||
| setOtlpEndpoint(environment.otlp_settings?.endpoint || ''); | |||
| setOtlpHeaders(environment.otlp_settings?.headers || {}); | |||
There was a problem hiding this comment.
caveat: headers are reordered by key automatically. I didn't want to make the model and code more complicated for such a tiny feature and the fact that in most case there will be only 1 header max
| @@ -0,0 +1,53 @@ | |||
| import { z } from 'zod'; | |||
There was a problem hiding this comment.
having one endpoint per setting is a bit insane imo
what do you think of taking this opportunity to create a generic endpoint (that only accepts otlp for now)?
There was a problem hiding this comment.
it is a bit insane. I also think the design is now overwhelming with all those options and I would vote for not only a endpoints refactor but a UI redesign as well. cc @bastienbeurier
I would prefer to push this otlp export feature to completion though and not go into a refactor at this point.
| @@ -336,6 +336,10 @@ class EnvironmentService { | |||
| return db.knex.from<DBEnvironment>(TABLE).where({ id }).update({ hmac_key: hmacKey }, ['id']); | |||
| } | |||
|
|
|||
| async editOtlpSettings(environmentId: number, otlpSettings: { endpoint: string; headers: Record<string, string> } | null): Promise<DBEnvironment | null> { | |||
| return db.knex.from<DBEnvironment>(TABLE).where({ id: environmentId }).update({ otlp_settings: otlpSettings }, ['id']); | |||
| } | |||
There was a problem hiding this comment.
same for this, there are 20 update functions while only need one that does this
await db.knex.from<DBEnvironment>(TABLE).where({ id: environmentId }).update(updatedEnv);
| </form> | ||
| </div> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
Would be nice to have an example in the tooltip or better in the placeholder of the input
There was a problem hiding this comment.
good point. will do
|
|
||
| const bodyValidation = z | ||
| .object({ | ||
| endpoint: z.string(), |
| if (isEnterprise) { | ||
| return true; | ||
| } | ||
| return featureFlags.isEnabled('feature:otlp:account', account.uuid, false); |
There was a problem hiding this comment.
what about self hosted?
There was a problem hiding this comment.
it is supposed to be a scale plan only feature so they can only be enterprise or cloud. Am I missing something?
There was a problem hiding this comment.
it shouldn't be shown in the UI then
There was a problem hiding this comment.
Agreed. Just check with @bastienbeurier though and we decided to keep it as is for now (aka: failing unless enterprise or cloud with flag enabled) and improve it when making the design of this page better
TBonnin
force-pushed
the
tbonnin/nan-1976/otlp-settings
branch
from
282156a to
54281fa
Compare
|
let me know when you need a second review |
|
@bodinsamuel fixed the zod schema and the endpoint placeholder. |
TBonnin
force-pushed
the
tbonnin/nan-1976/otlp-settings
branch
from
54281fa to
492011f
Compare
TBonnin
force-pushed
the
tbonnin/nan-1976/otlp-settings
branch
from
492011f to
688c735
Compare
Describe your changes
Add environment settings to input OpenTelemetry endpoint and headers
Note:
OpenTelemetry export is not enabled for this accounterror. I am happy to also make a mention of it being a paid feature in the environment setting page. Let me know @bastienbeurierChecklist before requesting a review (skip if just adding/editing APIs & templates)