Eth c++ client structured logging #1169

LefterisJP · 2015-03-01T00:51:41Z

Implementing the Structured Logging Spec for the C++ client.

Some fields are missing since I am not 100% familiar with that part of the codebase but I am positive with some quick feedback they will filled in fast.
At the moment starting the client with ./eth/eth --structured-logging will enable structured logging.
A nice bonus is that with the --structured-logging-format <%Y-%m-%d> we can give a strftime() type of format string to influence the way timestamps are printed.
I took the liberty of adding a Stop log event when the client shuts down. I think it would be needed.
Finally I apologize for "infecting" the PR with style fixes but I tried to follow the style guide as I was touching the code here and there.

subtly · 2015-03-02T03:00:18Z

libdevcore/StructuredLogger.cpp

+	if (m_enabled)
+	{
+		Json::Value event;
+		event["comment"] = "one of the first log events, before any operation is started";


Afaik this isn't implemented in the client and can be removed.

subtly · 2015-03-02T04:03:36Z

This CLI parameters are okay. Instead of creating a new class for this please look at the existing logging implementation and build on those. Would recommend creating a StructuredLogOutputStream similar to LogOutputStream; remove operator<<, add a log(string&,unsigned) and log(string&,string&), and replace m_sstr with Json::Value m_event. Set fwd declration for Json::Value in Log.h and implement log() in Log.cpp so we don't have to change includes in every file.

LefterisJP · 2015-03-02T10:17:57Z

I can understand the thread safety argument, but I really strongly believe that it should stay a separate class as it is right now.

I really don't like the idea of making another LogOutputStream. These 2 different kinds of logging systems serve totally different purposes. I will add a mutex to the current class to protect the output as the one in LogOutputStream does.

LefterisJP · 2015-03-02T10:49:30Z

rebased

LefterisJP · 2015-03-02T13:05:54Z

The 2 fields missing from the logging for now are:

chain head hash from eth.miner.new_block event
remote id from eth.chain.received.new_block event

gavofyork · 2015-03-02T14:53:24Z

libdevcore/StructuredLogger.h

+
+	void starting(std::string const& _clientImpl, const char* _ethVersion) const;
+	void stopping(std::string const& _clientImpl, const char* _ethVersion) const;
+	void p2pConnected(std::string const& _id, bi::tcp::endpoint const& _addr,


style is incorrect. see latest CodingStandards.txt, 0.a and the golden rule.

LefterisJP · 2015-03-02T23:29:21Z

Can anyone try to build this locally? I have no idea why buildbot fails to find json/json.h. It makes no sense. We use it in other files too.

onepremise · 2015-03-03T01:35:33Z

I've seen this happen before with builds which reference jsoncpp in nonstandard path. I had to explicitly include jsoncpp before /usr/include because it was getting confused with distros that have the standard jsonc library installed, "-I/include/jsoncpp -I/include". You could try changing '#include json/json.h" to "#include jsoncpp/json/json.h". That is if the build machine was recently updated and jsoncpp is now in the path it's usually installed at on most distributions.

LefterisJP · 2015-03-03T08:32:36Z

I see. Thanks for the info @onepremise. I'd rather not have to do black magic to appease the buildbot though.

Found it out. Seems CMakeLists.txt needed the jsoncpp headers location specified too. In my machine jsoncpp headers were added in /usr/include so no special config was required. The build should be fixed for the PR now.

- Also style fixes when problems encountered

- Structured logger is injected as a dependency where needed. - Still a work in progress

… getter.

onepremise · 2015-03-03T14:43:57Z

 I'd rather not have to do black magic to appease the buildbot

LefterisJP, wasn't trying to point this out as a 'black magic' fix. It really is a distribution issue. Most linux distros will have installs which deploy to /usr/include/json and /usr/include/jsoncpp. If you include in your source files, '#include json/json.h', there's no guarantee that others will be able to build your project without encountering this same issue. I know i did when i started building cpp-ethereum. They will have to explicitly know to include jsonccp first in their include path before this succeeds, that is if they have both jsonc and jsoncpp installed. Many open source projects have deferred to referencing jsoncpp in their source files to avoid this issue. Others, using cmake, have updated their FindJsoncpp.cmake file to search for features.h instead of json.h:

http://stackoverflow.com/questions/18005880/how-to-writing-a-cmake-module-for-jsoncpp

Luckily your buildbot environment isolates the issue.

LefterisJP · 2015-03-03T14:53:57Z

This is why we have cmake. The fix was to adjust the cmakefile for libdevcore. All seems to be sorted.

I did not want any discrepancies between source files. All of our jsoncpp includes should be json/json.h and let cmake sort out the rest.

gavofyork · 2015-03-03T15:13:18Z

libwebthree/WebThree.h

@@ -103,7 +103,10 @@ class WebThreeDirect : public WebThreeNetworkFace
 public:
 	/// Constructor for private instance. If there is already another process on the machine using @a _dbPath, then this will throw an exception.
 	/// ethereum() may be safely static_cast()ed to a eth::Client*.
-	WebThreeDirect(std::string const& _clientVersion, std::string const& _dbPath, bool _forceClean = false, std::set<std::string> const& _interfaces = {"eth", "shh"}, p2p::NetworkPreferences const& _n = p2p::NetworkPreferences(), bytesConstRef _network = bytesConstRef(), int miners = -1);
+	WebThreeDirect(std::string const& _clientVersion, std::string const& _dbPath, bool _forceClean = false,
+				   std::set<std::string> const& _interfaces = {"eth", "shh"},


style (one indent only)

gavofyork · 2015-03-03T15:19:10Z

style fixes needed. simple rule: if an expression goes on multiple lines, it's always:

PRE
    PART1 SEP
    PART2 SEP
    PART3 SEP
    ...
POST

note, this is not:

PRE PART1 SEP
    PART2 SEP
    PART3 SEP
    ...
POST

nor

PRE PART1 SEP PART2 SEP
    PART3 SEP ...
POST

SEP is ,, ; &c.

PARTs are arguments, initialiser lines &c.

gavofyork · 2015-03-03T16:01:11Z

libwebthree/WebThree.h

@@ -103,7 +103,14 @@ class WebThreeDirect : public WebThreeNetworkFace
 public:
 	/// Constructor for private instance. If there is already another process on the machine using @a _dbPath, then this will throw an exception.
 	/// ethereum() may be safely static_cast()ed to a eth::Client*.
-	WebThreeDirect(std::string const& _clientVersion, std::string const& _dbPath, bool _forceClean = false, std::set<std::string> const& _interfaces = {"eth", "shh"}, p2p::NetworkPreferences const& _n = p2p::NetworkPreferences(), bytesConstRef _network = bytesConstRef(), int miners = -1);
+	WebThreeDirect(
+		std::string const& _clientVersion, std::string const& _dbPath,


two items on one line

arrggh sorry

gavofyork · 2015-03-04T10:31:39Z

libdevcore/StructuredLogger.cpp

+	}
+}
+
+void StructuredLogger::chainReceivedNewBlock(string const& _hash,


still wrong.

Eth c++ client structured logging

gavofyork added the in progress label Mar 1, 2015

subtly reviewed Mar 2, 2015
View reviewed changes

subtly added the gotissues label Mar 2, 2015

LefterisJP force-pushed the eth_StructuredLogging branch from dc90863 to 5b28246 Compare March 2, 2015 10:49

LefterisJP added pleasereview and removed gotissues labels Mar 2, 2015

gavofyork reviewed Mar 2, 2015
View reviewed changes

LefterisJP force-pushed the eth_StructuredLogging branch from f8b4b3c to 17281c1 Compare March 2, 2015 23:27

LefterisJP force-pushed the eth_StructuredLogging branch from f72ae5c to e44ab45 Compare March 3, 2015 14:36

LefterisJP added 11 commits March 3, 2015 15:37

Adding argument for structured logging and propagating it forward

5695d0e

- Also style fixes when problems encountered

Adding StructuredLogger class and using it in various places

d0c1c51

- Structured logger is injected as a dependency where needed. - Still a work in progress

StructuredLogger propagated as a const reference

ba81493

Add p2p disconnected structured log event

643499e

Add remote version to p2pconnected log event

6c4faef

Structured logging should now work for some events

063a12f

StructuredLogging timestamp format is now configurable

97ab2d6

Adding a StructuredLogging stop event

afaac3f

Adding a mutex to StructuredLogger

d3ce888

StructuredLogger becomes a global singleton class

623d640

Adding number of connections in the p2p events

0fba1cc

LefterisJP added 4 commits March 3, 2015 15:37

Style fixes

ae6510b

StructuredLogger's functions all become static and call the singleton…

4a9b2fa

… getter.

mini style improvement

aabce85

fixing the build

e44ab45

gavofyork reviewed Mar 3, 2015
View reviewed changes

LefterisJP force-pushed the eth_StructuredLogging branch from a2cb12d to 5f191f4 Compare March 3, 2015 16:12

Indentation style fixes

5f191f4

gavofyork reviewed Mar 4, 2015
View reviewed changes

libdevcore/StructuredLogger.cpp

}

}

void StructuredLogger::chainReceivedNewBlock(string const& _hash,

Copy link

Contributor

gavofyork Mar 4, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still wrong.

More style fixes

3874c78

gavofyork pushed a commit that referenced this pull request Mar 4, 2015

Merge pull request #1169 from LefterisJP/eth_StructuredLogging

10839cf

Eth c++ client structured logging

gavofyork merged commit 10839cf into ethereum:develop Mar 4, 2015

gavofyork removed the in progress label Mar 4, 2015

LefterisJP mentioned this pull request Mar 5, 2015

Index access for calldata and byte arrays. #1212

Merged

LefterisJP mentioned this pull request Apr 15, 2015

implement structured logging in cpp-ethereum #705

Closed

LefterisJP deleted the eth_StructuredLogging branch June 24, 2015 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eth c++ client structured logging #1169

Eth c++ client structured logging #1169

LefterisJP commented Mar 1, 2015

subtly Mar 2, 2015

subtly commented Mar 2, 2015

LefterisJP commented Mar 2, 2015

LefterisJP commented Mar 2, 2015

LefterisJP commented Mar 2, 2015

gavofyork Mar 2, 2015

LefterisJP commented Mar 2, 2015

onepremise commented Mar 3, 2015

LefterisJP commented Mar 3, 2015

onepremise commented Mar 3, 2015

LefterisJP commented Mar 3, 2015

gavofyork Mar 3, 2015

gavofyork commented Mar 3, 2015

gavofyork Mar 3, 2015

LefterisJP Mar 3, 2015

gavofyork Mar 4, 2015

Eth c++ client structured logging #1169

Eth c++ client structured logging #1169

Conversation

LefterisJP commented Mar 1, 2015

subtly Mar 2, 2015

Choose a reason for hiding this comment

subtly commented Mar 2, 2015

LefterisJP commented Mar 2, 2015

LefterisJP commented Mar 2, 2015

LefterisJP commented Mar 2, 2015

gavofyork Mar 2, 2015

Choose a reason for hiding this comment

LefterisJP commented Mar 2, 2015

onepremise commented Mar 3, 2015

LefterisJP commented Mar 3, 2015

onepremise commented Mar 3, 2015

LefterisJP commented Mar 3, 2015

gavofyork Mar 3, 2015

Choose a reason for hiding this comment

gavofyork commented Mar 3, 2015

gavofyork Mar 3, 2015

Choose a reason for hiding this comment

LefterisJP Mar 3, 2015

Choose a reason for hiding this comment

gavofyork Mar 4, 2015

Choose a reason for hiding this comment