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(logs): Loki integration #108

Merged
merged 8 commits into from
Apr 28, 2024
Merged

feat(logs): Loki integration #108

merged 8 commits into from
Apr 28, 2024

Conversation

voximity
Copy link
Collaborator

@voximity voximity commented Apr 9, 2024

Loki integration to track agent node logs.

  • Basic log uploading to Loki
  • Pass location of Loki into agents
  • Authentication reverse proxy (or another form of authentication)
  • Sample Grafana integration

@voximity voximity marked this pull request as draft April 9, 2024 02:45
Signed-off-by: Zander Franks <[email protected]>
@voximity voximity marked this pull request as ready for review April 11, 2024 02:43
@voximity
Copy link
Collaborator Author

I'm undrafting this because it is not feature-complete in terms of auth and Grafana, but it is functional, and if we want it merged in before running any number of Canarynet nodes, we can merge it in early.

@voximity voximity mentioned this pull request Apr 12, 2024
Comment on lines +220 to +224
thread::spawn(|| {
let rt = tokio::runtime::Runtime::new().unwrap();
let handle = rt.spawn(task);
rt.block_on(handle).unwrap();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This might cause a panic. Could happen when we run the node and it delegates a tokio task to this thread, but it already has a runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about how to deal with this... Might switch to futures::executor::block_on

@@ -47,8 +45,7 @@ pub fn init(state: Arc<GlobalState>) {
.await
{
Ok(response) => response,
Err(e) => {
warn!("failed to scrape latest metrics: {e}");
Copy link
Contributor

Choose a reason for hiding this comment

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

We no longer want this warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can re-add once we have something that checks if the node is actually running. At the moment it is always spam

@@ -93,7 +92,7 @@ pub async fn log_request(uri: Uri, method: Method, req_stamp: ReqStamp, res: Res
};

// TODO: send to logging services
debug!("REQUEST LOG LINE:\n{}", json!(log_line));
// debug!("REQUEST LOG LINE:\n{}", json!(log_line));
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep this. it's useful for debugging, we can drop it down to a trace level if preferred.

We should also send these to loki so all requests to the cp are logged for tracing purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's very noisy with Prometheus running as it checks the service discovery API every few seconds. Could forward to Loki though

Copy link
Contributor

Choose a reason for hiding this comment

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

I could add a filter to ignore the Loki spam too


AOT_BIN="$(pwd)/target/$PROFILE/snarkos-aot" \
AGENT_BIN="$(pwd)/target/$PROFILE/snops-agent" \
cargo watch -x 'run -p snops -- --prometheus http://127.0.0.1:9090 --loki http://127.0.0.1:3100' \
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should make some of these things an ENV var in the future so you can just have .env file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea

@Meshiest
Copy link
Contributor

Meshiest commented Apr 28, 2024

merging this for two reasons:

  • can skip auth in favor of including the token in a subdomain: <longhash>.mywebsite.com
  • need it

@Meshiest Meshiest merged commit 64db819 into main Apr 28, 2024
2 checks passed
@Meshiest Meshiest deleted the feat-loki branch April 28, 2024 04:25
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