Feature request: Consider protected rather than private methods

[danrivett]

Use case

Following on from #1198, it would be fairly trivial to implement disabling a logger by extending the Logger and overriding the shouldPrint function. Unfortunately it's marked as a private method and so can't be.

This ticket has been created to evaluate using the protected method modifier instead of private. There's pros and cons to doing that, but in my experience it is a quick option for improving extensibility, and provides escape hatches.

There are other solutions that can be better for extensibility, but they normally require a larger investment by the library creator, whereas using the protected modifier is simple for the library creator, but puts the onus on the sub-class implementor to know exactly how that method is implemented and maintain any customization going forward.

Therefore another option is to selectively mark certain methods which are simple in nature and likely candidates for customization (such as shouldPrint above).

Solution/User Experience

Use the protected access modifier instead of private either generally for all private methods, or certain methods where it's likely a user may want to customize its behaviour.

Alternative solutions

No response

Acknowledgment

• This feature request meets Lambda Powertools Tenets
• Should this be considered in other Lambda Powertools languages? i.e. Python, Java

[dreamorosi]

Hi @danrivett, thank you for opening this issue.

While I see your point about extensibility, I'm concerned this could become an issue for both users and maintainers in the long term.

At the moment our commitment around stability and backwards compatibility includes only public APIs. By marking a certain method as private, we are intentionally signalling that users should not rely on its implementation and that it could change, potentially in a breaking way, without any notice.

If we were to allow extending any internal method we'd risk potentially causing production outages to anyone doing that, and I fear that this would constrain development excessively.

My current position is that I'd much rather continue listening to specific feature requests and evolve the library organically based on discussion/demand (like we are doing for #1198), rather than move to a free-for-all approach.

Before deciding either way, I'd still like to hear some additional arguments though

[danrivett]

Thanks for considering @dreamorosi. I understand your thought process, and I think there is a lot of merit there.

My personal opinion is that it should be quite clear (and documented as such) that only public methods are API methods which have the stability guarantees you outline, and any protected method extended is subject to change in any future release. It is up to the customizer to pin the library version they are using, and upgrade when they have a chance to evaluate that their customizations are still compatible with the newer library version.

This definitely introduces risk though, so I can see why we'd want to lock down private methods. In general though I'm in favour of allowing a sub-class to override non-public API methods because they sometimes want to decorate or customize functionality in ways that aren't anticipated by the library creator, or appropriate for a wider audience.

But these discussions have a lot of nuance, and I think what you say makes a lot of sense too.

That's perhaps why a middle ground of thinking of extensibility as the library is created, and marking certain methods as protected may be a good middle ground - I do think shouldPrint is a great example of a method that could well be protected in that case.

Protected methods wouldn't have the same backwards compatibility guarantees, and so if someone decides to sub-class and override a protected method, then they accept that.

I'd also think extending a class will be a rather niche option that is likely not done very often, so isn't going to impact the majority of users.

That said we did extend Logger previously to add a new logAnyError method to allow us to wrap a particular function call and have it intercept any error thrown, log it, and then rethrow the error, as sometimes we want to record errors being thrown by certain functions even if we ultimately handle the error.

// const result = await riskyExternalAPIFunction(functionArg1, functionArg2);
const result = await logger.logAnyError(riskyExternalAPIFunction, [functionArg1, functionArg2]);

This is another use case for being able to disable a particular logger (#1198) as we may decide to temporarily disable that particular logger at some point before we decide to remove the usage of logAnyError.

(Btw we implemented it as):

public async logAnyError<F extends (...input: any[]) => Promise<any>>(
    fn: F,
    args: Parameters<F>,
    debugMessage?: string,
    debugInfo?: LogAttributes
  ): Promise<ReturnType<F>> {
    this.debug(`Calling: ${fn.name}`, { args });
    try {
      return await fn(...args);
    } catch (error) {
      const msg = debugMessage || `An error occurred calling: ${fn.name}`;

      this.error(msg, { args, error, debugInfo });

      throw error;
    }
  }

That's probably a niche option too, and so thinking of extensibility to allow for things like this, is definitely beneficial, and avoids having to use things such as patch-package as an option of last resort.

If you ultimately decide that the cons outweigh the pros of introducing protected methods, I understand though, and thank you for your consideration at least, since it allows for a good discussion and evaluation. In that case, please feel free to close this ticket.

[danrivett]

A follow on from my previous comment, that I just ran into: We're starting to use child loggers with our extended Logger (CustomLogger in the example below):

export class CustomLogger extends Logger {
  public constructor(options?: LoggerOptions) {
    super(options);

    this.logAnyError = this.logAnyError.bind(this);
  }

  public async logAnyError<F extends (...input: any[]) => Promise<any>>(
    fn: F,
    args: Parameters<F>,
    debugMessage?: string,
    debugInfo?: LogAttributes
  ): Promise<ReturnType<F>> {
    this.debug(`Calling: ${fn.name}`, { args });
    try {
      return await fn(...args);
    } catch (error) {
      const msg = debugMessage || `An error occurred calling: ${fn.name}`;

      this.error(msg, { args, error, debugInfo });

      throw error;
    }
  }
}

Problem we're running into is we'd like to override the createChild() method to introduce a covariant return where it returns a CustomLogger instance and not a Logger instance like the super class does.

The problem we're finding is that the createChild() function news up the Logger instance directly (as is very typical of course):

...
    const childLogger = new Logger(merge(parentsOptions, parentsPowertoolsLogData, options));
...

A possible solution to this would be to introduce a protected factory method which created the Logger instance, and which createChild() would delegate to. Then our sub-class could override that to create a sub-class instance.

// In Logger.ts
protected createLogger(options?: ConstructorOptions): Logger {
  return new Logger(options);
}

// In CustomLogger.ts
protected createLogger(options?: ConstructorOptions): CustomLogger { // Covariant return
  return new CustomLogger(options);
}

createChild(options?: ConstructorOptions): CustomLogger { // Covariant return
  return super.createChild(options) as CustomLogger;
}

Then the custom logAnyError() method is available not just on the root logger, but also any child logger created.

There may be better approaches here, but this is my initial thought when trying to call logger.createChild() and running into issues with it not being a CustomLogger type unlike its parent.

[robertbeal]

Just came across this as I was looking to raise such a suggestion myself.

I was looking to extend Logger.ts - so that I could buffer debug logs and only flush them if an error/critical happened to give additional context. We don't want a LogLevel.DEBUG as it's too noisy/costly. But we would like DEBUG logs if an error/critical happened as they can add additional context.

But not being able to access a handful of properties means extending the class isn't possible. Or at least I'd have to re-write parts making my extended class far more brittle than I'd feel comfortable.

• It's not possible to see the current logLevel as it's private. So one can't infer anything based on what it's set to. I wouldn't see this as an unreasonable prop to expose (as read-only), given the plethora of ways to set log level.
• I'd need to be able to printLog and createAndPopulateLogItem to make a debug log still print as the log level would be > DEBUG. However, I can understand exposing these methods even as protected isn't really a good idea.

Considering some props/functions as protected as part of extending the Logger (and effectively it's API) would allow the wider community to further evolve and play with the powertools Logger. This is a bigger topic and design decision though.

[dreamorosi]

Thank you both for the detailed messages.

Since it's tangentially related to the discussion, I'd like to share that I have just merged a PR that introduces two new methods and a property to Logger that will allow you to get the current log level both as a number (logger.level // returns 12) and as a string (logger.getLevelName() // returns "INFO"), as well as setting a new log level at runtime (logger.setLogLevel('DEBUG')).

I also have changed the printLog method to protected following the discussion above. This should help with additional customization.

As mentioned above, rather than making all private methods protected by default, I'd rather evaluate specific feature requests on a case by case basis. This will allow us to consider the different tradeoffs between exposing a private method/property, adding a new feature, or allowing customization whenever the scope of the method is self-contained enough.

In this case, as pointed out earlier in the thread, the printLog is a relatively simple method and thus a good candidate for being made protected. On the other hand, other parts like the ones related to the log level have more cross-cutting impact across different methods, and so we opted for exposing public APIs that still expose the values and allow customization.

Once the changes mentioned above have been released (~1week from now), I propose to close this issue and resume the conversation on new discussions dedicated to each specific use case. This way we will be able to keep the discussion focused to a specific use case or feature and hopefully reach a conclusion that satisfies the most people.

[danrivett]

Thanks for the update and explanation @dreamorosi.

The only thing on my end that is outstanding is evaluating whether Logger could avoid newing up Logger instances inline, but instead use a factory method that can be overridden by sub-classes.

This would allow a Logger to be subclassed and any child loggers created to also be subclass instances.

I had a comment above which explained the rationale in more detail.

[dreamorosi]

Hi @danrivett, could you please provide an example of the factory method you mentioned? Would it be possible to subclass createChild directly instead?

[github-actions]

This issue has not received a response in 2 weeks. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

[danrivett]

Sorry for the delayed response @dreamorosi.

The problem I see with directly sub-classing the createChild() method is it has a lot of unrelated business logic (see here) other than instantiating a new Logger instance which is the thing we want to replace with a sub-class.

We don't want the sub-class to duplicate this as it's not DRY, but also is an upgrade issue when the super-class updates the business logic changes in createChild() in a future release.

Even the instantiation of new Logger in createChild() contains business logic because it contains a call to merge, but there's a lot of other business logic in that method.

So instead if we introduce a simple factory method that instantiates the Logger, then the sub-class can just override that method to instantiate sub-class but leave all the business logic in createChild() to the Logger class.

In terms of an example, I expanded on the one in a comment above which should illustrate what I mean:

// In Logger.ts

public createChild(options: ConstructorOptions = {}): Logger {
  ...
  // Replace new Logger() with createLogger()
  const childLogger = createLogger(
    merge(parentsOptions, parentsPowertoolsLogData, options)
  );
  ...
}

protected createLogger(options?: ConstructorOptions): Logger {
  return new Logger(options);
}

// The in our CustomLogger.ts sub-class we could simply implement:
protected createLogger(options?: ConstructorOptions): CustomLogger { // Covariant return
  return new CustomLogger(options);
}

// But to also have callers to the super createChild() method have the correct return type (our sub-class) we override it and just call the super method, but with a covariant return to return the correct type
public createChild(options?: ConstructorOptions): CustomLogger { // Covariant return
  return super.createChild(options) as CustomLogger;
}

[github-actions]

This issue has not received a response in 2 weeks. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

[dreamorosi]

Hi @danrivett apologies for the late reply on this.

Thank you for providing more context into the request, I don't see any issues with extracting the log creation into its own protected method if that makes customers life easier when subclassing the module.

I'm going to open a PR to do it however like mentioned in my initial response to this thread we can't guarantee that internal methods will stay the same. We won't be changing intentionally, but if a future refactor calls for it, we might do so without considering it a breaking change.

[github-actions]

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

[github-actions]

This is now released under v1.13.0 version!