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

Is that possible to config log destination to stdout in container environment #795

Closed
sherwinwangs opened this issue May 28, 2019 · 25 comments · Fixed by #836
Closed

Is that possible to config log destination to stdout in container environment #795

sherwinwangs opened this issue May 28, 2019 · 25 comments · Fixed by #836
Labels
area/logging Issues or PRs related to logging of Sentinel kind/discussion For further discussion

Comments

@sherwinwangs
Copy link

Issue Description

Type: feature request

Describe what happened (or what feature you want)

In container environment, we desire output log to stdout, so that we can collection log with EFK, but I cannot find a config option

Describe what you expected to happen

Output log to stdout

How to reproduce it (as minimally and precisely as possible)

Tell us your environment

Kubernetes

Anything else we need to know?

@sczyh30 sczyh30 added the kind/question Category issues related to questions or problems label May 28, 2019
@jasonjoo2010
Copy link
Collaborator

@sczyh30 @cdfive @CarpenterLee

We can figure out whether a configurable option should be introduced to register a different log handler to DateFileLogHandler in LogBase because stateless is the core concept for a container.

Btw, though there is already a similar handler ConsoleHandler but some customizing should be made due to it support only System.err. While it makes more sense dividing logs into System.out and System.err.

@sczyh30
Copy link
Member

sczyh30 commented May 29, 2019

Yes, it might be necessary to do this. There's another discussion here: #757

BTW, there are also metrics log and sentinel-block.log which are not implemented with the log handler mechanism. Considerations should be taken with the three kinds of logs.

@jasonjoo2010
Copy link
Collaborator

Yes, it might be necessary to do this. There's another discussion here: #757

BTW, there are also metrics log and sentinel-block.log which are not implemented with the log handler mechanism. Considerations should be taken with the three kinds of logs.

In this scenario metric log and block log are kinds of temporary storage offering api accessing. So it can be considered to be stateless and different from logging.

More opinions? @CarpenterLee @cdfive

@jasonjoo2010
Copy link
Collaborator

More on supporting common interface (eg. slf4j-api) again we can introduce a dependency with provided scope to make it work in various environments.

@cdfive
Copy link
Collaborator

cdfive commented May 29, 2019

I'm agree with that it's necessary to support log to stdout and make it configurable.
If print the log info to stdout, it will be convenient for users to see the detail info of Sentinel with their application(when started in local IDE env), and also convenient for log collecting(in container env).

Btw, though there is already a similar handler ConsoleHandler but some customizing should be made due to it support only System.err.

@jasonjoo2010 I also find that in ConsoleHandler class, if adding a new MyConsoleHandler class.

public static class MyConsoleHandler extends ConsoleHandler {
    public MyConsoleHandler() {
        super();
        setOutputStream(System.out);
    }
}

Use that Handler, we can print the log to stdout.

While it makes more sense dividing logs into System.out and System.err.

In container env, it distinguishes these two, or only stdout?

How about adding a config parameter like csp.sentinel.log.stdout=true?
The configurable option shoud be easy to configured.

make the logging interface as SPI and provide different implementations (based on JUL or slf4j)

@sczyh30 Does it means that the logging interface are used three kinds of logs?
I'm a little confused about that whether the LogBase should be still used or modified to support.

@jasonjoo2010
Copy link
Collaborator

In container env, it distinguishes these two, or only stdout?

Generally both of them can be collected and can be treated differently. So it's better to obey the real meaning of out and err.

Using SPI maybe make it too complicated and i haven't a full plan on it. But i think adding one more support is considerable(slf4j handler when it can be invoked or by configuration).

@sczyh30 sczyh30 added area/logging Issues or PRs related to logging of Sentinel kind/discussion For further discussion and removed kind/question Category issues related to questions or problems labels May 31, 2019
@jasonjoo2010
Copy link
Collaborator

@cdfive @sczyh30

Hi folks, what's the plan? Is anyone taking charge on it?
Some discussions can't be ended with a conclusion and I think we should give one in time.

So,

  1. If SPI should be used.(Supposed yes)
  2. If we should make an SPI implementation based on console.

Any updated?

@sczyh30
Copy link
Member

sczyh30 commented Jun 12, 2019

Does it means that the logging interface are used three kinds of logs? I'm a little confused about that whether the LogBase should be still used or modified to support.

The logging interface is for common log (e.g. RecordLog and CommandCenterLog). Logging SPI might be useful as we could provide integrations with both default JUL and slf4j, but we need to take care of the backward compatibility with the legacy LogBase.

If we should make an SPI implementation based on console.

We can support the console log if necessary.

Any other suggestions? Do you think it's necessary to abstract the logging SPI? @cdfive @CarpenterLee

@cdfive
Copy link
Collaborator

cdfive commented Jun 12, 2019

How about just making the LogBase support log to console? with a parameter to config.
As the requirement of user is simple: Output log to stdout.
Abstract the logging SPI maybe difficult and few users may extend it.

@jasonjoo2010
Copy link
Collaborator

How about just making the LogBase support log to console? with a parameter to config.
As the requirement of user is simple: Output log to stdout.
Abstract the logging SPI maybe difficult and few users may extend it.

Actually i think we should consider two scenarios:

  1. Container
    Outputting into stdout/stderr is enough.

  2. Project having logging implementation
    Merging into the existed implementation(eg. log4j).
    This will need SPI.

So i suggest that why we consider it as a single PR? We can divide it into two PRs and maybe make more discussions or someone else take charge on the SPI one.

@sczyh30
Copy link
Member

sczyh30 commented Jun 13, 2019

So I suggest that why we consider it as a single PR? We can divide it into two PRs and maybe make more discussions or someone else takes charge on the SPI one.

Yes, it can be divided into separate PRs. For logging SPI if someone has a good design we could discuss here: #757

@jasonjoo2010
Copy link
Collaborator

So I suggest that why we consider it as a single PR? We can divide it into two PRs and maybe make more discussions or someone else takes charge on the SPI one.

Yes, it can be divided into separate PRs. For logging SPI if someone has a good design we could discuss here: #757

It seems we reach to a common sense on separating it and who is willing to contribute the first one? (Console output)

@cdfive Are you willing to? /::D

@jasonjoo2010
Copy link
Collaborator

Seem like everybody busy so i will take charge on it.

And see alibaba/spring-cloud-alibaba#707

Not only actual users but also SCA does more duplicated things(loading test, validation etc.) which already exists in sentinel-core. The main reason i think is still the logging related. Because they(SCA) want output some obvious content to indicate whether there was any problem when initializing sentinel module.

According this i think i still will make them (Console & SPI) in single PR. What's your opinion, @sczyh30 ?

@cdfive
Copy link
Collaborator

cdfive commented Jun 14, 2019

@jasonjoo2010 I'd like to try, but I've been very busy with my work recently, hope it will be better next month. Besides I haven't a clear idea at the moment about it. Are there any good resources for learning to solve this?

@jasonjoo2010
Copy link
Collaborator

@jasonjoo2010 I'd like to try, but I've been very busy with my work recently, hope it will be better next month. Besides I haven't a clear idea at the moment about it. Are there any good resources for learning to solve this?

SPI you can found everywhere in Sentinel like how InitFunc does.

Console output is just like the discussion above we divide the logging into <= WARN and >= INFO into stderr and stdout.

And the formal i imagined as:

sentinel-core
sentinel-log-console
sentinel-log-slf4j
sentinel-log-log4j(optional, maybe we needn't it now)

If we only have sentinel-core in our project the logging is remain just as what it does now. But if we want it directed into console(err, out) we just introduce an additional dependency sentinel-log-console and no other action need to be done. (same to slf4j)

That will solve all the problems we meet currently.

The core concept in SPI is a file will be placed into META-INF/services like com.alibaba.csp.sentinel.transport.CommandCenter in sentinel-transport-simple-http.

@cdfive
Copy link
Collaborator

cdfive commented Jun 14, 2019

@jasonjoo2010 Thanks detailed description. Are three log modules too many? How about only one log module, which can be used easily. Besides I'm not sure about if a log module is needed.Can we refer the way to support log some other projects does?

@jasonjoo2010
Copy link
Collaborator

@jasonjoo2010 Thanks detailed description. Are three log modules too many? How about only one log module, which can be used easily. Besides I'm not sure about if a log module is needed.Can we refer the way to support log some other projects does?

The point is, we should consider carefully introducing new configuration item, less configuration and more simple.

On the other hand using SPI mechanism we give a possibility to our users making their own implementations. After all it's difficult to make everyone feel satisfied, isn't it?

Finally i just think it will be more simple and clearer for us to make new modules or am i missing something? And we generally can keep the most code by this way in sentinel-core and put new codes into new modules separately.

@cdfive
Copy link
Collaborator

cdfive commented Jun 15, 2019

After all it's difficult to make everyone feel satisfied

Agree with it.

For simply solve this issue mentioned, I post a PR, which simply add a option to support log the common log to console, PTAL. @jasonjoo2010 @sczyh30

So it's better to obey the real meaning of out and err.

Yes, obey the meaning is better, but for simple maybe stdout is enough.

Besides, I found that the level of log in java.util.logging.Level is not same as log4j.
They are SEVERE, WARNING, INFO, CONFIG, FINE, FINER, FINEST.
It maybe need take into consideration.

And for abstract logging SPI support, it maybe complicated and we can divide another PR.

@jasonjoo2010
Copy link
Collaborator

After all it's difficult to make everyone feel satisfied

Agree with it.

For simply solve this issue mentioned, I post a PR, which simply add a option to support log the common log to console, PTAL. @jasonjoo2010 @sczyh30

Besides, I find that the level of log in java.util.logging.Level is not same as log4j.
They are SEVERE, WARNING, INFO, CONFIG, FINE, FINER, FINEST.
It maybe need take into consideration.

And for abstract logging SPI support, it maybe complicated and we can divide another PR.

Yes agree with your last words.
And prompt the levels in JDK's logging we can just take CONFIG as INFO and FINE* as VERBOSE in log4j or others in my opinion.

But I still suggest to separate logs into stderr and stdout for obvious meaning expression. And can you give some suggestions on it, @sczyh30 ?

PS: I found it hardly quoting the wrong name again if I use Eric to make the prompting. For sharing :D

@cdfive
Copy link
Collaborator

cdfive commented Jun 15, 2019

@jasonjoo2010 I add builder.append(record.getLevel().getName()).append(" "); in CspFormatter class to print the level of log info. We can separate the meaning according to it.

To print logs separate into stderr and stdout, I tried and found a way, I'll post it later.
Is it OK that print all warning level or above log info to stderr, and info level or below to stdout?

@cdfive
Copy link
Collaborator

cdfive commented Jun 15, 2019

Support separate logs into stderr and stdout is added.
Since the getLevelProperty of LogManager class(JDK logging) is package privilege access, it's not convenient to customized the config, but in most circumstances, we may use the default settings.

Besides, the System.err log in IDE console is red font, while the System.out is white.
In sentinel-dashboard SprintBoot application log, not all word of one line is colorful, which is better looking.

@jasonjoo2010
Copy link
Collaborator

@jasonjoo2010 I add builder.append(record.getLevel().getName()).append(" "); in CspFormatter class to print the level of log info. We can separate the meaning according to it.

To print logs separate into stderr and stdout, I tried and found a way, I'll post it later.
Is it OK that print all warning level or above log info to stderr, and info level or below to stdout?

I think that's ok and if users want to merge them they could redirect stderr into stdout simply.

Since the getLevelProperty of LogManager class(JDK logging) is package privilege access, it's not convenient to customized the config, but in most circumstances, we may use the default settings.

Currently some integrating like RecordLog only provide INFO and WARN. So I think that is not a big problem and at most it could be a configuration item. So maybe we could ignore it now. What do you think out?

@cdfive
Copy link
Collaborator

cdfive commented Jun 15, 2019

redirect stderr into stdout simply

This is a good thought. So for common logs, maybe it's enough, hope it will solve the desire this issue mentioned.

For logging SPI, I haven't much experiences and expecting for your more ideas. Maybe we can refer other project like Dubbo.

@jasonjoo2010
Copy link
Collaborator

redirect stderr into stdout simply

This is a good thought. So for common logs, maybe it's enough, hope it will solve the desire this issue mentioned.

For logging SPI, I haven't much experiences and expecting for your more ideas. Maybe we can refer other project like Dubbo.

Never mind we can finish the first PR.

@sczyh30
Copy link
Member

sczyh30 commented Jun 19, 2019

For further discussion of the logging interface, we could discuss here: #757

CST11021 pushed a commit to CST11021/Sentinel that referenced this issue Nov 3, 2021
…/en/ (alibaba#795)

* To add the English version of Examples of Ordered Messages in docs/en/

* Format the style using MD gramma.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logging Issues or PRs related to logging of Sentinel kind/discussion For further discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants