Hackathon - log drain #1373
Hackathon - log drain #1373
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
|
lkostrowski
added
the
skip changeset
| json: z.object({ | ||
| enabled: z.boolean().optional(), | ||
| url: z.string().optional(), | ||
| headers: z.string().optional(), |
There was a problem hiding this comment.
here we should add json validation, so it doesnt shoot anyone in the foot. it will be hard to notice missing " and it will fallback to {} causing auth errors. so we need custom validation, as simple as json.parse() in try/catch
| @@ -141,6 +141,8 @@ export const EditAvataxConfiguration = () => { | |||
| ); | |||
| } | |||
|
|
|||
| console.log("config", data.config); | |||
| import { AvataxConfig } from "../avatax-connection-schema"; | ||
| import { HelperText } from "./form-helper-text"; | ||
|
|
||
| export const LogsSettingsFragment = () => { |
There was a problem hiding this comment.
we should have it in shared packages IMO but I think first we can finish this PR, then plan to enable it in other apps → we will extract form into shared package
UI can be shared easily, but we need to make form decoupled from data source
| name="logsSettings.otel.enabled" | ||
| control={control} | ||
| label="OTEL transport" | ||
| helperText={<HelperText>Enable sending logs using OpenTelemetry protocol.</HelperText>} |
There was a problem hiding this comment.
we need to also:
- write that this is JSON HTTP standard (not be confused with proto)
- add some links to docs of open telemetry etc
| @@ -127,6 +139,11 @@ describe("CalculateTaxesUseCase", () => { | |||
|
|
|||
| instance = new CalculateTaxesUseCase({ | |||
| configExtractor: MockConfigExtractor, | |||
| publicLogDrain: new PublicLogDrainService([ | |||
| { | |||
| async emit(log: PublicLog): Promise<void> {}, | |||
There was a problem hiding this comment.
we should add test that checks if log drain emits when use case runs
| const sourceObjectType = payload.taxBase.sourceObject.__typename; | ||
| const sourceObjectId = payload.taxBase.sourceObject.id; | ||
| const orderOrCheckoutId = | ||
| payload.taxBase.sourceObject.__typename === "Checkout" | ||
| ? { checkoutId: sourceObjectId } | ||
| : { orderId: sourceObjectId }; | ||
| const saleorApiUrl = authData.saleorApiUrl; |
There was a problem hiding this comment.
we can wrap in private method of clarity
| }; | ||
|
|
||
| class TaxesCalculationFailedLog implements PublicLog<CheckoutOrOrderId> { | ||
| message = "Taxes calculation failed"; |
There was a problem hiding this comment.
if we have separate events for successful calculations for checkout and order, we should probably be consistent here too
| const payload = { | ||
| ...log, | ||
| traceId: spanContext?.traceId, | ||
| spanId: spanContext?.spanId, | ||
| isRemote: spanContext?.isRemote, | ||
| traceFlags: spanContext?.traceFlags, | ||
| traceState: spanContext?.traceState?.serialize(), | ||
| }; |
There was a problem hiding this comment.
maybe otel provides type for this?
| return value; | ||
| } | ||
|
|
||
| private _filterAndTruncateAttributes(attributes: Record<string, unknown>) { |
There was a problem hiding this comment.
we dont have to use _ . We use typescript and have private methods
| try { | ||
| headers = JSON.parse( | ||
| providerConfig.value.avataxConfig.config.logsSettings.otel.headers ?? "", | ||
| ); | ||
| } catch { | ||
| headers = {}; | ||
| } |
There was a problem hiding this comment.
we should likely use dedicated class that wraps config and exposes parsed config
Scope of the PR
Related issues
Checklist