Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

print errors when environment variable conversions to int or double fail #203

Closed

Conversation

ecourreges-orange
Copy link
Contributor

Signed-off-by: CI-Bot for Emmanuel Courreges emmanuel.courreges@orange.com

Which problem is this PR solving?

Probably should log a warning if value cannot be parsed. But it's not in scope for this PR.
Originally posted by @yurishkuro in #200

Short description of the changes

Log failure information on std::cerr for every conversion error of environment variable to int or to double.
The config is loaded from env when we don't have a logger yet, that's why there is no alternative to std:cerr which was already used in similar cases.

Signed-off-by: CI-Bot for Emmanuel Courreges <emmanuel.courreges@orange.com>
@AppVeyorBot
Copy link

Build jaeger-client-cpp 87 completed (commit 2f9a8771c9 by @)

@mdouaihy
Copy link
Contributor

mdouaihy commented Mar 2, 2020

Hi @ecourreges-orange,

Thank you for your efforts.

Instead of writing to an std::cerr, why not passing a logger that can be used to pass the relevant information?

@ecourreges-orange
Copy link
Contributor Author

Hi @ecourreges-orange,

Thank you for your efforts.

Instead of writing to an std::cerr, why not passing a logger that can be used to pass the relevant information?

I thought about that and then saw that the logger is initialized after the conf is read, which means that the logger is not available yet in the fromEnv method.
At least that's what I remember.

…nt-cpp

Signed-off-by: CI-Bot for Emmanuel Courreges <emmanuel.courreges@orange.com>
Signed-off-by: CI-Bot for Emmanuel Courreges <emmanuel.courreges@orange.com>
@AppVeyorBot
Copy link

Build jaeger-client-cpp 121 completed (commit 17cf6db45b by @)

@ecourreges-orange
Copy link
Contributor Author

Hi @ecourreges-orange,

Thank you for your efforts.

@mdouaihy I updated the code and it is now mergeable following your recent commits on ConfigTest.

I don't want to go overboard here and introduce interface changes to the Config classes, and I think the logger part can be dealt later in another PR.
Thanks.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants