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

✨ HttpLoggingInterceptor: allow to pass custom logger #470

Merged
merged 1 commit into from
Aug 5, 2023
Merged

✨ HttpLoggingInterceptor: allow to pass custom logger #470

merged 1 commit into from
Aug 5, 2023

Conversation

Maxr1998
Copy link
Contributor

@Maxr1998 Maxr1998 commented Aug 4, 2023

This function can then be overridden to use alternative logging implementations besides the default flutter logging.

This can be especially useful if you want to only write one (multiline) log entry per request, instead of one entry per line.

@techouse
Copy link
Collaborator

techouse commented Aug 4, 2023

It might be better to change the constructor in order to allow passing a custom Logger into it, i.e.

class HttpLoggingInterceptor implements RequestInterceptor, ResponseInterceptor {
  HttpLoggingInterceptor({this.level = Level.body, Logger? logger})
      : _logBody = level == Level.body,
        _logHeaders = level == Level.body || level == Level.headers,
        _logger = logger ?? chopperLogger;

  final Logger _logger;
  /// stuff ...
}

@Guldem your thoughts?

@Maxr1998
Copy link
Contributor Author

Maxr1998 commented Aug 4, 2023

Would be an option, but I'd prefer to have both in that case.

With the extra function I'd be able to collect these lines in a single string per request, and then log them out at once - reducing unwanted interleaving of other log lines, and generally giving more flexibility.

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #470 (b52e6ba) into develop (03f7bf7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #470      +/-   ##
===========================================
+ Coverage    93.70%   93.72%   +0.01%     
===========================================
  Files            8        8              
  Lines          445      446       +1     
===========================================
+ Hits           417      418       +1     
  Misses          28       28              
Files Changed Coverage Δ
chopper/lib/src/http_logging_interceptor.dart 96.00% <100.00%> (+0.08%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@techouse
Copy link
Collaborator

techouse commented Aug 4, 2023

Would be an option, but I'd prefer to have both in that case.

You can achieve the same by simply overriding the log method of Logger.

@Maxr1998
Copy link
Contributor Author

Maxr1998 commented Aug 4, 2023

I see, I think that would also work. Let me know what you prefer and I'll implement it 😁

@Guldem
Copy link
Contributor

Guldem commented Aug 4, 2023

Overriding the a function feels a bit hacky. In general I think its better make the logger configurable. This should be specific to chopper. Another option could be to create a ChopperLogRecord class which contains a bit more information so you would be able to create your own fine tuned logging.

For example:

class ChopperLogRecord {
const ChopperLogRecord(this.message, {this.request, this.response});

final Request? request;
final Response? response;
final String message;

@override
 String toString() => message;
 }

// The HttpLoggingInterceptor would log like this:
_logger.info(ChopperLogRecord('the message', request: request);
// or
_logger.info(ChopperLogRecord('the message', response: response);

// Do whats need to collect logging event from chopper.
 Logger.root.onRecord.listen((event) {
      if (event is ChopperLogRecord) {
        _collect(event);
      }
    });

The hard part is when to stop collecting and print the summary ofc. Maybe print everything when the request/response is different (still not fool proof since its async).

@Maxr1998
Copy link
Contributor Author

Maxr1998 commented Aug 4, 2023

I like this idea! For this PR, I think just changing the interceptor to accept a custom logger would be enough, but let me know if you want me to work on the second suggestion as well.

@Maxr1998 Maxr1998 changed the title HttpLoggingInterceptor: pass lines through extra write function ✨ HttpLoggingInterceptor: allow to pass custom logger Aug 4, 2023
@Guldem
Copy link
Contributor

Guldem commented Aug 4, 2023

I like this idea! For this PR, I think just changing the interceptor to accept a custom logger would be enough, but let me know if you want me to work on the second suggestion as well.

Also feel free to add the LogRecord suggestion 😄

@Maxr1998
Copy link
Contributor Author

Maxr1998 commented Aug 4, 2023

Also feel free to add the LogRecord suggestion smile

Alright, I'll do that in a separate PR.

@techouse techouse added the enhancement New feature or request label Aug 4, 2023
@techouse
Copy link
Collaborator

techouse commented Aug 4, 2023

LGTM

@techouse techouse merged commit 8dbde76 into lejard-h:develop Aug 5, 2023
6 checks passed
@Maxr1998 Maxr1998 deleted the logging-interceptor-write-function branch August 5, 2023 07:16
@techouse techouse mentioned this pull request Aug 7, 2023
@techouse
Copy link
Collaborator

techouse commented Aug 7, 2023

@Maxr1998 this has now been released in v7.0.2.

Feel free to add the ChopperLogRecord feature mentioned above in a separate PR whenever you have time.

@Maxr1998
Copy link
Contributor Author

Maxr1998 commented Aug 7, 2023

Sure, the last few days have been a bit busy but I'll get to it later this week.

@Maxr1998
Copy link
Contributor Author

You can achieve the same by simply overriding the log method of Logger.

I'm not 100% happy with this solution yet. Logger seems to be a somewhat special class that only has factory constructors, and thus it's not possible to extend it.

As a workaround, one has to implement it through a delegate pattern, which gets quite messy. Do you perhaps have a suggestion for a better alternative?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants