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

Make it work with recent versions of spdlog. #131

Closed
wants to merge 1 commit into from

Conversation

PiotrSikora
Copy link
Contributor

Recent versions don't have "notice" and "emerg" levels,
so change "notice" to "info" and "emerg" to "critical".

@mattklein123
Copy link
Member

This is a little problematic given that there are other info lines that in general we don't want to print during runtime (which is why default before was notice).

I guess my feeling here is to change both notice/emerg -> critical and make the default critical. I realize that in general "critical" is supposed to mean "exiting soon and can't continue", but I don't think it really matters that much.

Otherwise we are going to have to change a whole bunch of existing info log lines to debug most likely (which is also fine, but a bigger change).

Thoughts?

Recent versions don't have "notice" and "emerg" levels, so change
"notice" to "info" or "error" and "emerg" to "critical".

Signed-off-by: Piotr Sikora <[email protected]>
@PiotrSikora
Copy link
Contributor Author

I changed the default level (and mongo decoding error) to "error" level, which I think is the sanest solution... Unless you really want to display all those logs that were previously at "notice" level? FWIW, I don't get why spdlog removed "notice" level...

Also, I don't know the circumstances of all the errors, so could you please double-check whether any of those that I lowered from "notice" to "info" level should be at "error" level instead? Thanks!

@mattklein123
Copy link
Member

There might be a few things that we want to leave at "err" just for startup and shutdown logging. Let me take a look at this brach locally. Which SHA of spdlog are you using? just current master?

@PiotrSikora
Copy link
Contributor Author

@mattklein123
Copy link
Member

Did you have to make any build changes? I'm getting use of old style cast warnings as errors. Sorry for the delay trying to get this working now.

@mattklein123
Copy link
Member

It looks like cotire doesn't correctly listen to SYSTEM includes either.

@mattklein123
Copy link
Member

OK I got it working. It's going to require a few other changes. If it's OK with you I think I'm just going to open a new PR built on top of your changes. I will tag you.

@PiotrSikora
Copy link
Contributor Author

Hey Matt,
oops, sorry! I didn't touch the build system (we're are using different one).

Feel free to start a new PR.

@mattklein123
Copy link
Member

Closing in favor of #160

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
kyessenov pushed a commit to kyessenov/envoy that referenced this pull request Jan 28, 2020
wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this pull request Jan 23, 2021
…egate_cluster

zh-translation: /intro/arch_overview/upstream/aggregate_cluster.rst
jpsim pushed a commit that referenced this pull request Nov 28, 2022
…on android_binary (#131)

The primary goal this achieves is that it unlocks our ability to develop the Envoy mobile shim in kotlin rather than java.

The gist of this change is that `android_library`'s implicit `aar` doesn't flatten its transitive dependencies. When using the kotlin rules, the `kt_android_library` rule creates a few underlying libraries, because of this the `classes.jar` in the `aar` we built was empty. This change separately builds the underlying `*kt.jar` file, and replaces the `aar`'s `classes.jar` with the kotlin one.

envoyproxy/envoy-mobile#99

Signed-off-by: Alan Chiu <[email protected]>

For an explanation of how to fill out the fields, please see the relevant section
in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md)

Description: Enable kotlin library development
Risk Level: low
Testing: ci, local
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
…on android_binary (#131)

The primary goal this achieves is that it unlocks our ability to develop the Envoy mobile shim in kotlin rather than java.

The gist of this change is that `android_library`'s implicit `aar` doesn't flatten its transitive dependencies. When using the kotlin rules, the `kt_android_library` rule creates a few underlying libraries, because of this the `classes.jar` in the `aar` we built was empty. This change separately builds the underlying `*kt.jar` file, and replaces the `aar`'s `classes.jar` with the kotlin one.

envoyproxy/envoy-mobile#99

Signed-off-by: Alan Chiu <[email protected]>

For an explanation of how to fill out the fields, please see the relevant section
in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md)

Description: Enable kotlin library development
Risk Level: low
Testing: ci, local
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: JP Simard <[email protected]>
arminabf pushed a commit to arminabf/envoy that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants