telemetry: lift definitions out of aws-toolkit-vscode #324
Conversation
| }, | ||
| { | ||
| "name": "lambdaArchitecture", | ||
| "name": "architecture", |
There was a problem hiding this comment.
Renaming this because it is identical with the architecture type, which is used in several other metrics. Should be easy to update this in all toolkits @rli @awschristou
lambdaArchitecture is fairly new (added in feb 2022), where as architecture is from 2021
There was a problem hiding this comment.
The change is easy to update, but I don't understand the motivation. This breaks existing reporting and graphs that are set up against lambdaArchitecture.
Where was architecture was referenced, when you mention it is from 2021?
lambdaArchitecture was introduced as a deliberate choice, to avoid namespace collisions, should the next system require a different definition of architecture.
There was a problem hiding this comment.
lambdaArchitecture was defined in-product Sept 2021 (the same time as the VS Code Toolkit change). It was migrated over in Feb 2022. There was an internal discussion about this before adding lambdaArchitecture into the definitions.
There was a problem hiding this comment.
architecture has been used in
apigateway_invokeLocallambda_invokeLocalsam_initsam_attachDebugger(which will likely be removed)
, which are now being lifted into common (previously was local to vscode).
We will have outdated metrics whichever choice we make. Or I could add a redundant field (lambdaArchitecture) to those metrics.
I'll do that since there is concern about the current PR.
There was a problem hiding this comment.
Personally, I'd let both types exist but not add redundant fields. This is the only true non-breaking change from a consumer's perspective. Otherwise you will have fragmented the data-set. Which is fine if you're willing to migrate the data on the backend.
The fundamental problem here is that our telemetry design associates data types 1:1 with field names. This goes against most modern IDLs (think Smithy, OpenAPI, WSDL) as well as pretty much every strongly-typed programming language that supports interfaces.
Names of fields describe additional context applicable to the associated data type. But right now we cannot express that. The information is bound to the data type itself.
There was a problem hiding this comment.
True, but consistent names are rather important for building reports. The datastore doesn't care about types. So we should use consistent names. That is why I tried to improve consistency here...
Otherwise you will have fragmented the data-set. Which is fine if you're willing to migrate the data on the backend.
An alternative is to have both names and drop the deprecated one a year from now. That is what this PR aims to do.
There was a problem hiding this comment.
The datastore doesn't care about types. So we should use consistent names.
Our datastore actually does care about types, we just automatically make everything a string :)
But yeah consistent naming is important. It's just that our current design gets in the way of sharing types with the same semantics but not the same context.
An alternative is to have both names and drop the deprecated one a year from now. That is what this PR aims to do.
Yup I see what your intent was now. For some reason I thought we were going to have Toolkits emit one or the other, not both. If both are emitted, then that works.
justinmk3
force-pushed
the
jmk/telem
branch
2 times, most recently
from
8a4af65 to
1944d30
Compare
justinmk3
changed the title
| { "type": "reason", "required": false }, | ||
| { "type": "eventBridgeSchema", "required": false } | ||
| { "type": "eventBridgeSchema", "required": false }, | ||
| { "type": "architecture", "required": false }, |
There was a problem hiding this comment.
Should we just keep architecture confined to the VSC Toolkit? It's somewhat counter to the goal of this PR, though it may be less confusing for other Toolkits.
There was a problem hiding this comment.
Or even just give up on architecture entirely as I think it's just specific to VSC. We can fix the data on the backend as-needed.
There was a problem hiding this comment.
That makes sense. Because it's vscode-only, none of our main dashboards are using this field.
- Lift vscode metrics defs from aws/aws-toolkit-vscode#2102 - Backwards-compatible change: add redundant `lambdaArchitecture` field to metrics with `architecture` field
BREAKING CHANGE: renamelambdaArchitecturetoarchitecturelambdaArchitecturefield to metrics witharchitecturefieldLicense
I confirm that my contribution is made under the terms of the Apache 2.0 license.