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

Graphql logging #1746

Merged
merged 16 commits into from
Sep 18, 2024
Merged

Graphql logging #1746

merged 16 commits into from
Sep 18, 2024

Conversation

miratepuffin
Copy link
Collaborator

@miratepuffin miratepuffin commented Sep 4, 2024

What changes were proposed in this pull request?

  1. Fixed tracing within the graphql server
  2. Pushed logging throughout raphtory - removing all printlns
  3. Fixed a constant properties function in graphql which was pub instead of async

Why are the changes needed?

  • Logging and tracing make it a lot easier to see what is going on in the server

Does this PR introduce any user-facing change? If yes is this documented?

No

How was this patch tested?

Current test suite

Issues

Fixes #1697

Are there any further changes required?

Copy link
Collaborator

@ljeub-pometry ljeub-pometry left a comment

Choose a reason for hiding this comment

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

looks good, some minor formatting cleanup would be good.


async fn unique_layers(&self) -> Vec<String> {
self.graph.unique_layers().map_into().collect()
}
#[instrument(skip(self))]

Copy link
Collaborator

Choose a reason for hiding this comment

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

weird blank lines in this file (some places lost the blank line between functions and other have a blank line after the attribute like here)

from raphtory import Graph
from raphtory.graphql import GraphServer, RaphtoryClient

# TODO this doesn't work currently due to how we terminate with Tracing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have we fixed this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we can turn this test back on.

Some(tracer_provider)
}
Err(e) => {
error!("{}", e.to_string());
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we just move on if we were told to enable tracing but we couldn't?

@fabianmurariu
Copy link
Collaborator

This was a lot of work, but I can't help but thinking 90% of these println! should have been removed instead of replaced

Ben Steer added 3 commits September 18, 2024 16:01
# Conflicts:
#	raphtory-benchmark/src/common/mod.rs
#	raphtory/src/algorithms/motifs/temporal_rich_club_coefficient.rs
#	raphtory/src/serialise/serialise.rs
#	raphtory/src/vectors/template.rs
@miratepuffin miratepuffin merged commit b38d95f into master Sep 18, 2024
19 checks passed
@miratepuffin miratepuffin deleted the feature/graphql-logging branch September 18, 2024 16:32
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.

impl tracing in graphql, rid printlns
3 participants