Skip to content

refactor(log): improved logging with instance level logging enable feature.#136

Merged
mohitpubnub merged 11 commits into
masterfrom
CLEN-2820
Aug 6, 2025
Merged

refactor(log): improved logging with instance level logging enable feature.#136
mohitpubnub merged 11 commits into
masterfrom
CLEN-2820

Conversation

@mohitpubnub

@mohitpubnub mohitpubnub commented Aug 1, 2025

Copy link
Copy Markdown
Contributor

feature: support for disabling retry.

Added support for RetryPolicy.none() to disable retry for subscription.

BREAKING CHANGES: The default retry policy has been changed from applying to all operations to now applying only to the subscription feature, with the exponential policy set by default.

refactor: improved logging in the library

Enhanced logging using standardized logging practices.

@mohitpubnub mohitpubnub self-assigned this Aug 1, 2025
@mohitpubnub mohitpubnub added priority: medium This PR should be reviewed after all high priority PRs. type: refactor This PR contains refactored existing features. labels Aug 1, 2025

// Log module information
messageString += '\n\tModules:';
messageString +=

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that each module could override toString() method, which should make creating a final String representation of a PubNub object a bit easier:

messageString += pubnub.networking.toString()
messageString += pubnub.parser.toString()
messageString += pubnub.crypto.toString()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍🏻
applied mentioned approach at 65e82a7

for (var i = 0; i < allKeysets.length; i++) {
var keyset = allKeysets[i];
var name = keysetNames[i];
messageString += '\n\t $name:';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps the same as mentioned above can be applied to custom Keyset type:

https://github.com/pubnub/dart/pull/136/files#r2250969267

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same at 65e82a7

} else if (detailsType == LogEventDetailsType.networkRequestInfo) {
var requestMap = details as Map<String, dynamic>;

for (var entry in requestMap.entries) {

@jguz-pubnub jguz-pubnub Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It works, although I do think this may be challenging to maintain in the long-run. Do you have the ability to introduce and log more abstract types, let's say, NetworkRequest or NetworkResponse? They can have properties specific to request and response, respectively, and override toString() method. Then, you could always call .toString() to create your messageString regardless of the passed object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍🏻 updated at 65e82a7

Comment thread pubnub/lib/src/default.dart Outdated
}

/// Set up global logging that will be available throughout the entire SDK
static void _setupGlobalLogging(

@jguz-pubnub jguz-pubnub Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Let's leave it as it is now because of the scope of the change; however, it would be great to consider dependency injection tools like GetIt or even a custom one in the long-run

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree! There is scope for enhancement. And this can be part of a dedicated task which can deal with it along with other impacted components.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jguz-pubnub now with dff7080 change. We have a logging configuration injectable

Now the initialisation looks like:

        var pubnub = PubNub(
          defaultKeyset: Keyset(
              subscribeKey: 'sub-key',
              publishKey: 'pub-key',
              userId: UserId('test-user')),
          logging: LoggingConfiguration(
            logLevel: Level.all, // Enable logging at all level
            // optional other configurations.
            loggerName: 'test-pubnub',
            logToConsole: false, 
          ),
        );

bool includeMessageType = true,
bool includeCustomMessageType = false,
bool includeUUID = true}) async {
_logger.info('Fetch messages API call');

@jguz-pubnub jguz-pubnub Aug 4, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I assume any method like _logger object, like .info, or .fine produce a LogRecord you can listen to or print it to the console. Is this record's format compliant with the format we have in ADR? For example:

"${timestamp} PubNub-${instanceId} Debug ${callsite} Some message"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

here 'Fetch messages API call' is just a part of a message.
The final log information in it's string format (in default format) looks like this:
2025-08-05 07:37:29.303300 info PubNub-49078.pubnub.dx.batch: Fetch messages API call

Mohit Tejani added 2 commits August 5, 2025 08:50
…dule and types information in readable fomat and cleanup the logging implementation to use toString() methods on those modules.
…isable retry for subscribe

test(retry_policy): added tests for retry policy
refactor: retry policy applicable to subscribe only
@mohitpubnub

mohitpubnub commented Aug 5, 2025

Copy link
Copy Markdown
Contributor Author

@jakub-grzesiowski there is missing support for disabling retry. So adbfbd5 contains changes to handle disabling retry scenario with RetryPolicy.none()

if (details is Response) {
messageString += details.toString();
} else if (details is Map) {
var detailsMap = details as Map<String, dynamic>;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What happens if you call details.toString() regardless of the underlying object type? Why don't we want to print the whole Map object and select a few keys only to be printed out?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree!! The issue we have here in this specific case is:
We need to print request url along with response details and in networking module we are missing URL in case of dart:io 's HttpResponse..

However once we have more standard Networking, We can have more cleaner version here


late final ILogger __logger;
Fiber(this._core, {required this.action}) : id = _id++ {
Fiber(this._core, {required this.action, RequestType? requestType})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see we could apply dart format to update our code to follow the Dart formatting guidelines and enable it to run on each save. You don't have to do it now, I'm thinking about creating a story for that to see what's the outcome

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

before committing I ran dart format. Strange that this isn't formatted! Looking into it

…guration for logging at instance level.

Removed static methods from default file to have a clean approach.
@mohitpubnub

Copy link
Copy Markdown
Contributor Author

@pubnub-release-bot release dart as v6.0.0

@mohitpubnub mohitpubnub merged commit ffb5d5c into master Aug 6, 2025
6 checks passed
@mohitpubnub mohitpubnub deleted the CLEN-2820 branch August 6, 2025 10:54
@pubnub-release-bot

Copy link
Copy Markdown
Contributor

🛑 Build failed. Please check release build for details 🛑

@Ali-Toosi

Copy link
Copy Markdown

Just linking this issue to this PR: #137
Unless the SDK is planned to drop support for web, it appears dart:mirrors isn't available on web and v6.0.0 doesn't compile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: medium This PR should be reviewed after all high priority PRs. type: refactor This PR contains refactored existing features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants