Skip to content
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

feat: Add initial event schemas with Supported Event Types #2

Merged
merged 24 commits into from
Nov 15, 2024

Conversation

thogarty
Copy link
Collaborator

@thogarty thogarty commented Nov 5, 2024

  • Add:
    • Change data schema
    • Change Alert data schema
    • Metric data schema
    • Metric Alert data schema
    • SupportedEventTypes data loader file
    • Data Loader File Update Script to be added to GHA later

@thogarty thogarty requested a review from ryli17 November 7, 2024 01:24
@@ -0,0 +1,225 @@
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be consistent with the names here:
events / alerts vs change/changeAlerts

@@ -1,3 +1,199 @@
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add am/crh and NE sections

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catalog is for json validation only. Follows strict schema set in https://json.schemastore.org/schema-catalog

"description": "The time that the resource that fired the event was updated",
"format": "date-time"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResourceChange is not change log. Please refer to the following:

    change:
      uuid: 65e86e2f-328e-40d4-b47f-3a4327123f1b
      type: CONNECTION_UPDATE
      status: COMPLETED
      createdDateTime: '2024-11-11T17:30:54Z'
      data:
      - op: replace
        path: "/name"
        value: p2p1-update-2
        previousValue: p2p1-update-1

op can be one of [replace, add, remove]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented.

Comment on lines 71 to 91
"ResourceChange": {
"properties": {
"createdBy": {
"type": "string",
"description": "User account that created the resource that fired the event"
},
"createdDateTime": {
"type": "string",
"description": "The time that the resource that fired the event was created",
"format": "date-time"
},
"updatedBy": {
"type": "string",
"description": "User account that updated the resource that fired the event"
},
"updatedDateTime": {
"type": "string",
"description": "The time that the resource that fired the event was updated",
"format": "date-time"
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Comment on lines 7 to 20
"cloudeventTypes": [
"equinix.fabric.connection.bandwidth_rx.usage",
"equinix.fabric.connection.bandwidth_tx.usage",
"equinix.fabric.port.bandwidth_rx.usage",
"equinix.fabric.port.bandwidth_tx.usage",
"equinix.fabric.port.frames_erred_rx.count",
"equinix.fabric.port.frames_erred_tx.count",
"equinix.fabric.port.frames_dropped_rx.count",
"equinix.fabric.port.frames_dropped_tx.count",
"equinix.fabric.router.routes_ipv4_installed.utilization",
"equinix.fabric.router.routes_ipv6_installed.utilization",
"equinix.fabric.metro.{:asideMetroCode}_{:zsideMetroCode}.latency",
"equinix.fabric.metric"
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloudeventTypes for metrics is only one: equinix.fabric.metric
Can we add another metricNames list, and put metric name to it.

Comment on lines 72 to 76
"change": {
"$ref": "#/definitions/ResourceChange",
"additionalProperties": true,
"description": "Chang information for the resource that fired the event"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change block here for metric resource.

"properties": {
"type": {
"type": "string",
"description": "The type of the metric event"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be: The type of the metric

"metrics": {
"type": "array",
"items": {
"$ref": "#/definitions/Metrics",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The item itself should be named Metric here.

"title": "Resource",
"description": "Schema of the resource that fired the event"
},
"Metrics": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which means this should be defined as Metric. That is the reason I suggest to use Data at the top level.

},
"additionalProperties": true,
"type": "object",
"title": "Metrics",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metric

Comment on lines 13 to 15
"$ref": "#/definitions/MetricAlert",
"definitions": {
"MetricAlert": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, can we rename to Data?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

"properties": {
"type": {
"type": "string",
"description": "The type of the metric event"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove event. Please check similar descriptions.

Copy link
Contributor

@ryli17 ryli17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@thogarty thogarty merged commit 8b70c74 into main Nov 15, 2024
3 checks passed
@thogarty thogarty deleted the import_cloud_events branch November 15, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants