-
Notifications
You must be signed in to change notification settings - Fork 25
NETOBSERV-2609 show tls usage in topology view #1451
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?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,11 +79,18 @@ export interface TopologyMetricPeer { | |
| subnetLabel?: string; | ||
| } | ||
|
|
||
| /** TLS message types and protocol versions from Loki matrix metric labels (TLSTypes, TLSVersion), when present. */ | ||
| export type GenericMetricTls = { | ||
| types?: string[]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. types can be removed I guess?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed in 3ab6360 I initially kept that in the UI just in case but it seems we are going to a direction where it will never be the case. |
||
| versions?: string[]; | ||
| }; | ||
|
|
||
| export type GenericMetric = { | ||
| name: string; | ||
| values: [number, number][]; | ||
| stats: MetricStats; | ||
| aggregateBy: Field; | ||
| tls?: GenericMetricTls; | ||
| }; | ||
|
|
||
| export type FunctionMetrics = { | ||
|
|
@@ -198,6 +205,8 @@ export type TopologyMetrics = { | |
| values: [number, number][]; | ||
| stats: MetricStats; | ||
| scope: FlowScope; | ||
| /** TLSTypes / TLSVersion from Loki topology matrix labels when present. */ | ||
| tls?: GenericMetricTls; | ||
| }; | ||
|
|
||
| export type NamedMetric = TopologyMetrics & { | ||
|
|
||
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.
I'm not sure to understand why not just using the
appendTLSFilterdefined above. Did I miss an edge case there?Uh oh!
There was an error while loading. Please reload this page.
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.
Ah, I guess I get it: it's because you're doing an aggregation per TLS type, right?
I think we can simplify and forget about TLS types here; not only because imho it's not a super important thing to show in aggregated data, but also because I omitted it intentionally in prom metrics (for cardinality)
I think if we omit TLS types in aggregations, that simplifies the whole things quite a bit?
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.
Actually, TLSGroup is much more meaningful to have in aggregateed data. PQC compliance is based on TLSGroup.