-
Notifications
You must be signed in to change notification settings - Fork 66
Internal telemetry pipeline nodes config #1794
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1794 +/- ##
==========================================
- Coverage 84.33% 84.32% -0.01%
==========================================
Files 495 495
Lines 144350 144422 +72
==========================================
+ Hits 121740 121789 +49
- Misses 22076 22099 +23
Partials 534 534
🚀 New features to boost your workflow:
|
| /// A collection of nodes forming a pipeline graph. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, Default)] | ||
| #[serde(transparent)] | ||
| pub struct PipelineNodes(HashMap<NodeId, Arc<NodeUserConfig>>); |
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.
Same comment as in the other PR:
nit: We can do something more "structured" using named attributes instead of a tuple
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 tried to make this type transparent to serde, which works (IIUC) for tuples.
|
|
||
| # Note the internal and nodes graphs are separate. | ||
| # They do not share a namespace. | ||
| internal: |
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.
Also copied from the other PR:
Suggestion: We can make it multi-graph with the same effort and risk.
Something like:
graphs: # array of graphs
- internal: # name of the graph
# other graph level properties, including the logger type
nodes: # copied from the original graph structure
telemetry: # same block that is configured here
...We can keep the existing configuration of single graph as is. This would be more an extension of the configuration.
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.
Agree. I will defer to Laurent who has in mind a use-case for pipeline groups.
| /// across multiple cores/threads without cloning the entire configuration. | ||
| nodes: HashMap<NodeId, Arc<NodeUserConfig>>, | ||
| #[serde(default)] | ||
| nodes: PipelineNodes, |
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 update the doc comments on the lines above to not mention Arc<NodeUserConfig> now. We should mention that for pub struct PipelineNodes(HashMap<NodeId, Arc<NodeUserConfig>>);
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.
Done
| /// Internal telemetry pipeline nodes. These have the same structure | ||
| /// as `nodes` but are independent and isolated to a separate internal | ||
| /// telemetry runtime. | ||
| #[serde(default, skip_serializing_if = "PipelineNodes::is_empty")] | ||
| internal: PipelineNodes, |
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 have an issue with this approach. Even though, as things stand, our engine only supports a single pipeline configuration, it is designed to support multiple pipelines (with different config) running in parallel within the same process. In the configuration crate, we have this logical grouping called PipelineGroup, whose long term goal is to group multiple pipelines that belong to the same group, for example a group could be mapped to a tenant.
All of this is to say that we will have multiple pipeline configurations running at the same time, and therefore a single internal telemetry system (ITS) receiving internal telemetry from different pipeline configurations. Because of that, it does not seem ideal to start tying the ITS configuration to a specific pipeline configuration. In the long run, I agree that we will need a way to define pipeline specific processing for our internal telemetry, but I propose that we address this in a second phase.
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 have no strong opinion on how we organize the graph as far as where the internal telemetry definition belongs.
| // Validate internal pipeline if present | ||
| if !self.internal.is_empty() { | ||
| self.internal | ||
| .validate(pipeline_group_id, pipeline_id, &mut errors); |
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.
This could lead to some inconvenience in some cases of validation errors. For example, if the main pipeline and the internal pipeline have nodes with the same name and there is a validation error, it wouldn't be clear if the error belongs to the main pipeline section or the internal pipeline section.
Maybe we can add a bool field is_internal to Context or just append "internal" to the pipeline_id when validating internal pipeline?
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 appended _internal.
utpilla
left a comment
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.
Left a suggestion about ambiguity related to error source.
lquerel
left a comment
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.
Before to merge this PR, I'd like to discuss this comment.
#1794 (comment)
lquerel
left a comment
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.
After discussing with @jmacd , we decided to merge this PR as is. I will work on another PR to reposition the engine's general configuration so that it is independent from the pipeline configurations themselves.
632c9be
Part of #1771.
Part of #1736.
Follows #1741.
This moves the HashMap<_, _> of nodes into a struct
PipelineNodesand re-uses it to parse an identical graph ofinternalnodes. This internal graph will be use when an internal logging provider is configured to output to an internal pipeline.