-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Monitor] harden async try/catch #12563
Conversation
This PR looks great but can we wait until after #12417 goes in so we don't accidentally lose the changes? |
…re/monitor/harden-tests
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.
Changes look good, just a couple comments.
const { _response: res } = await this._appInsightsClient.track(envelopes); | ||
return { statusCode: res.status, result: res.bodyAsText ?? "" }; | ||
} catch (e) { | ||
return { statusCode: 0, result: e?.message ?? "Error serializing envelope" }; |
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.
do we have an alias for what statusCode: 0
means?
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.
Added StatusCode.Undefined
to Constants
. The ingestion server will never return this code and so I don't think it makes sense to move it to codegen, but it feels awkward having a one-off alias here. WDYT?
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 mean how is this code going to be documented so that the user can figure out what it means? Is there a different StatusCode
that makes sense 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 status code is just meant to be a non-retriable error, but I'm not sure if another HTTP code make sense here. An OTel code could be used, but they are (were) grpc based, so they don't map well. Alternatively, I can just rethrow here and let the caller handle it
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.
Ohhhh it's an HTTP status code. I'm OK with 0 for this, other runtimes have used that for weird internal errors.
sdk/monitor/opentelemetry-exporter-azure-monitor/src/platform/nodejs/persist/noopPersist.ts
Outdated
Show resolved
Hide resolved
Some CI failues:
|
@xirzec: Could we get these fixes tagged in a release on NPM? Thanks! |
Enable python track2 pipeline (Azure#12563)
No description provided.