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

Additional attributes for log messages #1026

Merged
merged 19 commits into from
Jun 15, 2016
Merged

Conversation

sergeuz
Copy link
Member

@sergeuz sergeuz commented Jun 11, 2016

Github doesn't allow to change base branch for existing PRs, so I'm recreating #1022 after base branch of the latter PR has been merged to develop.

This PR is rebased on top of current develop.

Original message:

This PR implements LOG_ATTR() macro which allows to specify additional attributes for log messages. For example:

wiced_result_t result = wiced_init();
if (result != WICED_SUCCESS) {
    LOG_ATTR(ERROR, (code = result, detail = "wiced"), "Initialization error");
}

int error = mbedtls_rsa_gen_key(&rsa_ctx, rng_func, 1024, 65537);
if (error) {
    LOG_ATTR(ERROR, (code = error, detail = "mbedtls"), "Unable to generate keys");
}

Only code and detail attributes are supported at the moment. New attributes (e.g. thread ID), can be easily added by extending LogAttributes structure.

The implementation involves a bit of preprocessor magic in order to support named parameters syntax in both C and C++, so this patch also adds small header file with some typical preprocessor primitives (see services/inc/preprocessor.h). In principle, the same could be easily implemented using Boost.Preprocessor, I just wasn't sure if we want to introduce additional compile-time dependency for the firmware build.

Note that this patch is based on the feature/logger_api branch and introduces ABI breaking changes to the existent dynalib interface of the logging library. It is likely too late to include this patch into upcoming 0.6.0, so I would mark current logging API as "experimental and subject to change" in the develop and postpone publishing of the logging API reference until next release.


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

@sergeuz sergeuz added this to the 0.6.x milestone Jun 11, 2016
@sergeuz
Copy link
Member Author

sergeuz commented Jun 11, 2016

From the original PR:

any thoughts on how the log attributes can be added to the Wiring logging API?

We have multiple options here, starting with the similar named parameters syntax implemented via overloaded operator= (unusual and requires quite a bit of boilerplate code). More traditional Java-like approach could look like the following:

Log.info(Log.attributes().setCode(-1).setDetails("details"), "message");

I'm kind of ambivalent about adding such functionality though. Most of the attributes are automatic and cannot be extended easily by user. As for the code and detail attributes, those are going to be used for CLI-assisted log analysis (at least we had some plans for that) and we probably don't want application logging to interfere with it. Also, low-level logging API which supports attributes, source code info and some other stuff is still available for applications, and it's not that scary to use if necessary :)

@m-mcgowan
Copy link
Contributor

Ok, we could make it a parameter, which get's a bit confusing with nesting...how about using a more fluid style of coding?

Log.attributes().setCode(-1).setDetails("details").info("message");

So the attributes class for a log subsumes the logger and exposes the same interface.

@sergeuz
Copy link
Member Author

sergeuz commented Jun 11, 2016

Looks good 👍

@sergeuz
Copy link
Member Author

sergeuz commented Jun 12, 2016

Added user API with the following syntax:

Log.code(-1).details("details").info("message");


```cpp
// EXAMPLE
Log("This is %s message", "info");
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the "info" parameter in case someone confuses it with actually setting the logging level. Something like

Log("The secret of everything is %d", 42);

@m-mcgowan
Copy link
Contributor

If the _v functions are only used at compile time, do we need them exposed as a dynalib API? If not, I'd like to remove it so we can save the space used!

@sergeuz
Copy link
Member Author

sergeuz commented Jun 14, 2016

If the _v functions are only used at compile time, do we need them exposed as a dynalib API? If not, I'd like to remove it so we can save the space used!

There's no other way to forward C-style ... variable arguments to another function other than using va_list, and __attribute__((format(...))) doesn't work with C++ variadic template functions. Please check the services/src/logging.cpp file, where both regular and _v functions are defined – basically, variants of the functions taking variable arguments via ... are simple wrappers over the same functions taking va_list, so the space overhead shouldn't be a concern.

Also, va_list variants of the logging functions allow to build module's internal logging functions on top of them, if necessary.

@sergeuz
Copy link
Member Author

sergeuz commented Jun 15, 2016

I think this should be ready for merging, @m-mcgowan?

I've moved the formatting code in question to the StreamLogHandler class for now. In general, LogHandler and StreamLogHandler classes are never used directly in application code (unless it implements its own log handler), nor documented in the firmware reference, so we still have some space for refactoring if it will be necessary.

@m-mcgowan m-mcgowan merged commit 2e923c7 into develop Jun 15, 2016
@technobly technobly deleted the feature/logging_attr branch October 27, 2016 17:24
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