feat: Add edge observability #713
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
chriswk
force-pushed
the
feat/edgeObservability
branch
from
6e19fe1 to
466eba7
Compare
chriswk
changed the title
chriswk
commented
Feb 10, 2025
Comment on lines
+124
to
+127
| .with_boundaries(vec![ | ||
| 1.0, 5.0, 10.0, 20.0, 30.0, 40.0, 50.0, 100.0, 200.0, 300.0, 400.0, 500.0, 750.0, | ||
| 1000.0, 1500.0, 2000.0, | ||
| ]) |
There was a problem hiding this comment.
this one used 0.0 as its first bucket, which since we're operating with time, we don't have negative numbers.
chriswk
force-pushed
the
feat/edgeObservability
branch
from
337e957 to
e064c77
Compare
sighphyre
reviewed
Feb 13, 2025
sighphyre
reviewed
Feb 13, 2025
server/src/http/instance_data.rs
Outdated
| .unleash_client | ||
| .send_instance_data(observed_data, &instance_data_sender.token) | ||
| .await; | ||
| match status { |
There was a problem hiding this comment.
Personal taste but I think if let Err is a nice construct than an empty Ok match. You could replace this with
if let Err(e) = status {
match e {
EdgeError::EdgeMetricsRequestError(status, message) => {
warn!("Failed to post instance data with status {status} and {message:?}");
if status == StatusCode::NOT_FOUND {
debug!("Upstream edge metrics not found, clearing our data about downstream instances to avoid growing to infinity (and beyond!).");
empty = true;
do_the_work = false;
} else if status == StatusCode::FORBIDDEN {
warn!("Upstream edge metrics rejected our data, clearing our data about downstream instances to avoid growing to infinity (and beyond!)");
empty = true;
do_the_work = false;
}
}
_ => {
warn!("Failed to post instance data due to unknown error {e:?}");
empty = false;
}
}
}
sighphyre
reviewed
Feb 13, 2025
sighphyre
reviewed
Feb 13, 2025
server/src/http/instance_data.rs
Outdated
| loop { | ||
| let mut empty = true; | ||
| tokio::time::sleep(std::time::Duration::from_secs(60)).await; | ||
| if let Some(instance_data_sender) = instance_data_sender.clone() { |
There was a problem hiding this comment.
So this function is pretty nested. Again personal opinion, but if we never stored the data in if we couldn't send it, then we could use
let Some(instance_data_sender) = instance_data_sender.clone() else {
continue;
};
to flatten this out a little
sighphyre
reviewed
Feb 13, 2025
| } | ||
| } | ||
|
|
||
| pub async fn send_instance_data( |
There was a problem hiding this comment.
Can we give this a different name? We now have two functions called the same thing doing different things
sighphyre
reviewed
Feb 13, 2025
| Ok(()) | ||
| } else { | ||
| match result.status() { | ||
| StatusCode::BAD_REQUEST => Err(EdgeMetricsRequestError( |
There was a problem hiding this comment.
Because copy-pasta from earlier request. I still like it though, because it exposes full message if we receive 400 from upstream, so we know what we need to fix.
sighphyre
reviewed
Feb 13, 2025
server/src/metrics/edge_metrics.rs
Outdated
| pub p99: f64, | ||
| } | ||
|
|
||
| impl LatencyMetrics { |
There was a problem hiding this comment.
Default is already implemented for this struct, this shouldn't be needed
sighphyre
reviewed
Feb 13, 2025
server/src/metrics/edge_metrics.rs
Outdated
| started: Utc::now(), | ||
| traffic: InstanceTraffic::default(), | ||
| latency_upstream: UpstreamLatency::default(), | ||
| connected_edges: Vec::new(), |
There was a problem hiding this comment.
Not married to it but I'm so used to seeing the macro invocation over explicit new here
Suggested change
| connected_edges: Vec::new(), | |
| connected_edges: vec![], |
chriswk
force-pushed
the
feat/edgeObservability
branch
from
f3060e5 to
c802591
Compare
Draft state: get latency for own endpoints from prometheus get latency for upstream endpoints from prometheus get process stats from prometheus instantiate on startup
chriswk
force-pushed
the
feat/edgeObservability
branch
from
9b8636c to
1f2778d
Compare
This was referenced Feb 20, 2025
Closed
Closed
Closed
Closed
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.