[Monitor][Codegen] Use generated TelemetryItem (as Envelope) #11280
Conversation
ghost
added
the
| envelope.data.baseData.measurements = { | ||
| ...envelope.data.baseData.measurements, | ||
| baseData.type = `Queue Message | ${namespace}`; | ||
| (baseData as any).source = `${peerAddress}/${messageBusDestination}`; |
There was a problem hiding this comment.
baseData is currently generated as an empty interface for each Envelope type. I need to investigate whether the TS extension or the swagger file should be improved to make these nicer to work with
There was a problem hiding this comment.
I'd hope we can generate something better than empty! @srnagar - any thoughts on how the swagger can be improved?
sdk/monitor/monitor-opentelemetry-exporter/src/utils/spanUtils.ts
Outdated
Show resolved
Hide resolved
| envelope.data.baseData.measurements = { | ||
| ...envelope.data.baseData.measurements, | ||
| baseData.type = `Queue Message | ${namespace}`; | ||
| (baseData as any).source = `${peerAddress}/${messageBusDestination}`; |
There was a problem hiding this comment.
I'd hope we can generate something better than empty! @srnagar - any thoughts on how the swagger can be improved?
sdk/monitor/monitor-opentelemetry-exporter/src/utils/spanUtils.ts
Outdated
Show resolved
Hide resolved
…re/monitor/codegen/migrate-envelope
|
@xirzec @markwolff: I'm using this in staging. Any chance we can publish the latest preview to npm? I'm looking forward to metrics exporting. Keep up the good work! |
…re/monitor/codegen/migrate-envelope
…re/monitor/codegen/migrate-envelope
There was a problem hiding this comment.
Some thoughts -- mostly I'm worried that our public shape isn't very well defined. I feel like we could better express the shapes that go into / come out of this library better with TypeScript than we are. If that's a codegen limitation, we could also consider hand-authoring some of the shapes.
| @@ -23,7 +23,7 @@ export interface TelemetryItem { | |||
| /** | |||
| * Event date time when telemetry item was created. This is the wall clock time on the client when the event was generated. There is no guarantee that the client's time is accurate. This field must be formatted in UTC ISO 8601 format, with a trailing 'Z' character, as described publicly on https://en.wikipedia.org/wiki/ISO_8601#UTC. Note: the number of decimal seconds digits provided are variable (and unspecified). Consumers should handle this, i.e. managed code consumers should not use format 'O' for parsing as it specifies a fixed length. Example: 2009-06-15T13:45:30.0000000Z. | |||
| */ | |||
| time: Date; | |||
| time: string; | |||
There was a problem hiding this comment.
did we really mean to make this a string and not a Date? Usually Date is easier to work with.
There was a problem hiding this comment.
Agreed. Have you gotten dates to be cross-lang interoperable in other client SDKs? Maybe the mapper/serializer could handle mapping Date to string?
There was a problem hiding this comment.
Doesn't it work if you do something like this?
"startTime": {
"type": "string",
"format": "date-time",
"description": "The time when an indexer should start running."
}
| */ | ||
| test?: string; | ||
| [property: string]: any; |
There was a problem hiding this comment.
This means anything that mixes in with MonitorDomain isn't type-checked by TS as all properties are valid. It would be best to avoid this if possible, since it's easy to make typos.
There was a problem hiding this comment.
This can go away once autorest supports oneOf polymophism. Until then, this is meant to be a temporary workaround
sdk/monitor/monitor-opentelemetry-exporter/src/platform/nodejs/httpSender.ts
Outdated
Show resolved
Hide resolved
| break; | ||
| default: // no op | ||
| } | ||
| } | ||
|
|
||
| return envelope; | ||
| return envelope as Envelope; |
There was a problem hiding this comment.
All the casts in this function seem fragile, since we're basically opting in and out of types.
Would it be possible to assemble the envelope at the end to avoid the partial? And perhaps to type all the base data separately before assigning to the envelope? It's a little tricky, but I feel like there should be a way to assemble a syntactically valid Envelope without having to turn off typechecks.
|
|
||
| assert.strictEqual(envelope.data?.baseData?.source, undefined); | ||
| assert.strictEqual(envelope.data?.baseData?.measurements, undefined); | ||
| const baseData = envelope.data?.baseData as RemoteDependencyData; |
There was a problem hiding this comment.
how do you imagine folks actually using this? I submit if they have to do their own casts, we're not providing much value to TS.
There was a problem hiding this comment.
Agreed. The baseData types won't be very useful until we can use some oneOf schemas in the swagger file
…/httpSender.ts Co-authored-by: Jeff Fisher <jeffish@microsoft.com>
…:Azure/azure-sdk-for-js into feature/monitor/codegen/migrate-envelope
…re/monitor/codegen/migrate-envelope
TelemetryItem(asEnvelope) interface instead .bond fileEnvelopeclass