Validation for required attributes #865
Conversation
| } | ||
| } | ||
| return m | ||
| } | ||
|
|
||
| func NewMetadata(attrs Attributes) *Metadata { | ||
| m := &Metadata{} | ||
| m := &Metadata{Timestamp: time.Now().UTC()} |
There was a problem hiding this comment.
@nanchano, We should try not to use attributes with unbounded cardinality.
It can cause various problems see this thread for details
https://chainlink-core.slack.com/archives/C06TEMFRPQD/p1729082258236499
There was a problem hiding this comment.
I could be wrong, but the unbounded cardinality issue is a problem for metrics, not for custom events, right?
There was a problem hiding this comment.
How would we wanna represent a timestamp then? As a string and let clients parse it?
There was a problem hiding this comment.
It probably should be inside the custom message payload.
FYI there is Timestamp field exist in otel log record
https://opentelemetry.io/docs/specs/otel/logs/data-model/#log-and-event-record-definition
I could be wrong, but the unbounded cardinality issue is a problem for metrics, not for custom events, right?
yes, but attributes can be reused between different telemetry types e.g span attributes are copied to metric attributes.
not for custom events, right?
I depends how we are going to store it.
We can put it in the attribute and get back to it later.
Whats the use case for having it in attributes?
There was a problem hiding this comment.
Loki suffer from labels hight cardinality as well:
See also https://utcc.utoronto.ca/~cks/space/blog/sysadmin/GrafanaLokiCardinalityProblem
https://grafana.com/docs/loki/latest/get-started/labels/#cardinality
This is high cardinality, and it can lead to significant performance degredation.
When we talk about cardinality we are referring to the combination of labels and values and the number of streams they create. High cardinality is using labels with a large range of possible values, such as ip, or combining many labels, even if they have a small and finite set of values, such as using status_code and action.
High cardinality causes Loki to build a huge index and to flush thousands of tiny chunks to the object store. Loki currently performs very poorly in this configuration. If not accounted for, high cardinality will significantly reduce the operability and cost-effectiveness of Loki.
There was a problem hiding this comment.
Right, but custom events are not sent to Loki, so the cardinality issue is at play here, I think
| @@ -22,9 +23,9 @@ type Metadata struct { | |||
| // The version of the CL node. | |||
| NodeVersion string | |||
| // mTLS public key for the node operator. This is used as an identity key but with the added benefit of being able to provide signatures. | |||
| NodeCsaKey string | |||
| NodeCsaKey string `validate:"required"` | |||
There was a problem hiding this comment.
@4of9, are these going to be added as resource attributes to beholder config (since they are static) or we want to pass them as regular attributes?
There was a problem hiding this comment.
I was thinkin NodeCsaKey would be included as a resource attribute so users don't have to add it to every message
There was a problem hiding this comment.
Same applies to NodeCsaSignature
There was a problem hiding this comment.
We might want to add validation for resource attributes as well.
E.g check if NodeCsaKey is set then NodeCsaSignature should be set as well
| "beholder_data_schema": "/schemas/ids/1001", // Required field, URI | ||
| "beholder_data_schema": "/schemas/ids/1001", // Required field, URI | ||
| "node_csa_key": "node_csa_val", // Required | ||
| "node_csa_signature": "mode_csa_signature_val", // Required |
There was a problem hiding this comment.
I don't think this would be an attribute, the signature will be coming through the authentication headers
|
closing. |
Rules: