-
Notifications
You must be signed in to change notification settings - Fork 257
Document 'gcp.apphub_destination.*' attributes. #2663
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
base: main
Are you sure you want to change the base?
Document 'gcp.apphub_destination.*' attributes. #2663
Conversation
brief: > | ||
The name of the destination service as configured in AppHub. | ||
examples: ['my-service'] | ||
- id: gcp.apphub_destination.service.environment_type |
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.
the rule of thumb is to use .
whenever it makes sense. It seems type is a property of env and (in theory) there could be multiple other properties, thus .
makes sense
- id: gcp.apphub_destination.service.environment_type | |
- id: gcp.apphub_destination.service.environment.type |
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.
While this would be a reasonable suggestion in the initial design (when gcp.apphub.service.environment_type
was defined), renaming here would break the rule that "for every attribute gcp.apphub.*
there is an equivalent gcp.apphub_destination.*
field corresponding to the destination application".
Given that gcp.apphub.service.environment_type
already exists, I would like to push back on this request.
brief: > | ||
The name of the destination workload as configured in AppHub. | ||
examples: ['my-workload'] | ||
- id: gcp.apphub_destination.workload.environment_type |
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.
let's not define multiple attributes for the same thing.
According to docs the same enum is shared
Environment of the Application, Service, or Workload
so, can we define one and call it gcp.apphub?destination.environment.type
or similar?
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.
For similar reasons as the other comments, I need to push back.
I concur that there is a lot of overlap between the "service" and "workload" concepts in AppHub. Were I in charge, I would have made a single definition with "SERVICE" and "WORKLOAD" as enum types that are independent of the overall structure.
While it would have been ideal to have:
message AppHub {
AppHubApplication application
ApplicationComponentType component_type // WORKLOAD or SERVICE
string id
CriticalityType criticality_type
EnvironmentType environment_type
}
What actually exists in AppHub is:
message AppHub {
AppHubApplication application
oneof component_type {
AppHubService service
AppHubWorkload workload
}
}
... with AppHubService
and AppHubWorkload
having substantially similar fields. This organizational structure is carried through to the currently defined AppHub attributes (perhaps under the assumption that these fields may take on divergent meaning or values in the two contexts in the future).
The desired rule "for every gcp.apphub.*
attribute, there is an equivalent gcp.apphub_destination.* attribute corresponding to the destination application" ties my hands a bit in terms of improving this organization.
Thanks for the review, @lmolkova . I know that I pushed back on a number of the requests here... please let me know if you are OK with me resolving those conversations or if you still consider these to be blockers. Thanks! |
Fixes #2649
Changes
Updates the markdown docs related to GCP AppHub to document destination attributes.
Merge requirement checklist
[chore]