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

Better logging #490

Merged
merged 1 commit into from
Sep 12, 2024
Merged

Better logging #490

merged 1 commit into from
Sep 12, 2024

Conversation

johnml1135
Copy link
Collaborator

@johnml1135 johnml1135 commented Sep 12, 2024

Make it a bit cleaner and easier to parse.


This change is Reviewable

@johnml1135 johnml1135 requested a review from Enkidu93 September 12, 2024 13:48
Copy link
Collaborator

@Enkidu93 Enkidu93 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @johnml1135)


src/Serval/src/Serval.Shared/Controllers/ErrorResultFilter.cs line 12 at r1 (raw file):

        if (context.HttpContext.Response.StatusCode >= 400)
        {
            string routeData = JsonSerializer.Serialize(

Is there no cleaner way to do this? It seems like there ought to be a specific property you can compare against. And did you confirm that this works? (I expect it would - just confirming).


src/Serval/src/Serval.Translation/Controllers/TranslationEnginesController.cs line 708 at r1 (raw file):

            return NoContent();
        _logger.LogInformation(
            "Returning USFM for {TextId} in engine {EngineId} for corpus {corpusId}",

This should be helpful!

@ddaspit ddaspit requested a review from Enkidu93 September 12, 2024 14:51
Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Enkidu93 and @johnml1135)


src/Serval/src/Serval.Shared/Controllers/ErrorResultFilter.cs line 12 at r1 (raw file):

Previously, Enkidu93 (Eli C. Lowry) wrote…

Is there no cleaner way to do this? It seems like there ought to be a specific property you can compare against. And did you confirm that this works? (I expect it would - just confirming).

Do we need to log every request that responds with a 4XX status code? Most 4XX codes are pretty normal. Is there a subset of 4XX codes that we are interested in? Logging 5XX codes makes sense.

@johnml1135
Copy link
Collaborator Author

src/Serval/src/Serval.Shared/Controllers/ErrorResultFilter.cs line 12 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

Do we need to log every request that responds with a 4XX status code? Most 4XX codes are pretty normal. Is there a subset of 4XX codes that we are interested in? Logging 5XX codes makes sense.

I agree - let's just do 500 codes.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 57.08%. Comparing base (df6673e) to head (5515063).

Files with missing lines Patch % Lines
...src/Serval.Shared/Controllers/ErrorResultFilter.cs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
- Coverage   57.16%   57.08%   -0.08%     
==========================================
  Files         277      277              
  Lines       14152    14157       +5     
  Branches     1900     1900              
==========================================
- Hits         8090     8082       -8     
- Misses       5474     5487      +13     
  Partials      588      588              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ddaspit ddaspit left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Enkidu93)

@johnml1135 johnml1135 merged commit cf71555 into main Sep 12, 2024
2 of 3 checks passed
@johnml1135 johnml1135 deleted the better_logging branch September 12, 2024 17:19
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.

4 participants