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

Define a limit on the number of Loggers #1259

Closed
MarkSeufert opened this issue Nov 23, 2020 · 6 comments
Closed

Define a limit on the number of Loggers #1259

MarkSeufert opened this issue Nov 23, 2020 · 6 comments
Labels
release:after-ga Not required before GA release, and not going to work on before GA spec:logs Related to the specification/logs directory

Comments

@MarkSeufert
Copy link

What are you trying to achieve?
I propose that there should be a limit for the number of loggers that a user can create. This limit prevents poorly designed instrumented code from creating an arbitrary number of loggers and consuming an unbounded amount of memory. This is important for agreeing with the OTel performance guidelines, where it says that the “Library should not consume unbounded memory resource.” The limit will also be large enough that customer use cases aren’t affected.

Additional Context
A similar feature is defined inside of the tracing specification, where each tracer instance has a limit on the number of spans it is allowed to create.

cc - @alolita @xukaren

@MarkSeufert MarkSeufert added the spec:logs Related to the specification/logs directory label Nov 23, 2020
@anuraaga
Copy link
Contributor

Hi @MarkSeufert - can you clarify the limit you're thinking about? For reference, there's no limit on the number of "Tracer"s in an app, which roughly corresponds to Loggers. There also isn't a limit on spans (spans are controlled by sampling rate), but there is a limit on the amount of dynamic data (attributes, events) that can be added to a span during its lifetime. Since a log message doesn't have a lifetime, I guess this may not apply to log messages? Rather than on the Logger side, I would expect there to be limits on the exporter (async batch queue, for example) that doesn't allow unbounded queues of logs. I think this would be similar to our batch span processor - https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#batching-processor

@MarkSeufert
Copy link
Author

The limit I'm thinking of here would be for the number of Logger instances that can be created by the LoggerProvider. So for example, when the user calls the LoggerProvider.GetLogger(name) method to create a logger, there would be a count internally and if the user has already created a large number of loggers, like 10,000 for example, then the LoggerProvider wouldn't create a new logger instance.

So this issue isn't really related to the log messages that the user writes, because you're right that the batch processor ensures a max queue size for that, but it's more about if the user accidentally creates a very large number of logger instances and consumes an unbounded amount of memory.

Yea Tracers doesn't have this implemented right now either, but I'm thinking it's an edge case such that the OpenTelemetry library can't consume unbounded memory.

@anuraaga
Copy link
Contributor

@MarkSeufert I see, you mentioned the tracing specification so I thought you might be comparing to Tracers but got it. Tracers aren't cached in any map so I think generally we wouldn't limit the number of Tracers, while it's not idiomatic, it's not the end of the world for a user to accidentally create a Tracer per request for example. But in this case, the memory usage isn't unbounded as there's no residual state so I think usually we wouldn't enforce any limit.

Do Loggers have state that increases for every logger created that would indicate a leak if a user e.g., created a Logger every request? If not, I wouldn't worry about it really, there would only be unbounded memory if the user also stored those into a Map or something, but that's an explicit action by the user and not something we should be messing with. On the flip side, creating a Logger per request, while extremely rare, can have some use cases (I think I've needed to do that once for a special case before).

@MarkSeufert
Copy link
Author

Yea looking at the traces implementation in C++ they return the same instance of the tracer with every call to GetTracer() so it makes sense how a limit isn't needed there.

Currently though I'm working on the logging SDK for C++ and each logger instance that is created is stored inside a map with the name associated with it (see here). We're doing it like this so that if a user wants to get the same instance of the logger in a separate part of the code, they can call GetLogger(name) and the map will be checked to see whether it contains that logger already. Log4j does it like this so I'm trying to keep our implementation as parallel as possible to it.

@MarkSeufert
Copy link
Author

PS: I just saw that OTel's dotnet logging uses maps to store the loggers too.

@tigrannajaryan
Copy link
Member

The current aproach to logging is defined here and it is not yet clear what limits are necessary.

I am closing this since it may not be applicable anymore. We can reopen or file new issues once the prototyping is done (currently in progress with Java) and we know more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:after-ga Not required before GA release, and not going to work on before GA spec:logs Related to the specification/logs directory
Projects
None yet
Development

No branches or pull requests

4 participants