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

Add custom_dimensions using kwargs instead of args #837

Merged
merged 6 commits into from
Jan 15, 2020

Conversation

xinyi-joffre
Copy link
Contributor

@xinyi-joffre xinyi-joffre commented Jan 10, 2020

This PR allows users to add customDimensions to their logs for both traces and exceptions. This approach was discussed in #834 before implementing.

As a part of this shift, it will break the changes introduced as part of #822. However, the new implementation allows for a smoother transition for those wanting to keep existing logging functionality with string interpolation and args. In addition, this PR adds support for exceptions which was not covered as part of the previous implementation.

Usage:

import logging

from opencensus.ext.azure.log_exporter import AzureLogHandler

logger = logging.getLogger(__name__)
# TODO: set APPLICATIONINSIGHTS_CONNECTION_STRING as environment variable.
logger.addHandler(AzureLogHandler())

properties = {'custom_dimensions': {'key_1': 'value_1', 'key_2': 'value_2'}}

# Use properties in logging statements
logger.warning('action', extra=properties)

# Use properties in exception logs
try:
    result = 1 / 0  # generate a ZeroDivisionError
except Exception:
    logger.exception('Captured an exception.', extra=properties)

Once this data is uploaded to App Insights, you can search in the traces and exceptions table by keys included in the customDimensions column like this:

traces
| where custom_dimensions.key_1 == "value_1"
| limit 50
| order by timestamp desc
exceptions
| where custom_dimensions.key_1 == "value_1"
| limit 50
| order by timestamp desc

Finally, this PR also contains contextual logging examples, for those who need to add context to a large number of logs without needing to add the extra= keyword to each log statement.

@xinyi-joffre
Copy link
Contributor Author

The CircleCI tests seem to be failing on ModuleNotFoundError: No module named '_sqlite3'. I don't think I changed anything that would break the existing modules. Could this be an existing issue with the build? @lzchen

@xinyi-joffre
Copy link
Contributor Author

@lzchen, do you want to me also keep the deprecated args implementation as well? I can add it, and print out a warning message about new functionality

@lzchen
Copy link
Contributor

lzchen commented Jan 13, 2020

@lzchen, do you want to me also keep the deprecated args implementation as well? I can add it, and print out a warning message about new functionality
@xinyi-joffre
I think we want to introduce this as the proper method to be adding custom dimensions, so it's okay to make this a breaking change.

@lzchen
Copy link
Contributor

lzchen commented Jan 13, 2020

@xinyi-joffre
Looks good and thanks for contributing! I will be taking a look at the build errors.

@lzchen
Copy link
Contributor

lzchen commented Jan 13, 2020

@xinyi-joffre
Actually now that I think about it, introducing these classes in the examples might cause confusion to users (they might assume these exist as features). I think it will be better to remove this example file.

Copy link
Contributor Author

@xinyi-joffre xinyi-joffre left a comment

Choose a reason for hiding this comment

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

@xinyi-joffre
Actually now that I think about it, introducing these classes in the examples might cause confusion to users (they might assume these exist as features). I think it will be better to remove this example file.

Okay, no worries, I will remove the contextual_logging.py example

@xinyi-joffre
Copy link
Contributor Author

xinyi-joffre commented Jan 13, 2020

I also realized that opencensus also uses 'attributes' in tracing for contextual information. Do you like the custom_dimensions name (since it matches the Application Insights field name)? Or should I change it to something simpler like attributes, properties, etc.?

@lzchen
Copy link
Contributor

lzchen commented Jan 13, 2020

@xinyi-joffre
Since this feature is specific to Azure, I would prefer to use custom_dimensions. attributes is taken from span_attributes which is a concept more related to tracing, so it is fine for logging to use a different name (metrics uses tags to represent custom dimensions). Also, attributes is a common enough word that users might be using it for other things.

@lzchen
Copy link
Contributor

lzchen commented Jan 15, 2020

@xinyi-joffre
Can you rebase? I believe [#842] has fixed the build errors.

@lzchen lzchen closed this Jan 15, 2020
@lzchen lzchen reopened this Jan 15, 2020
@xinyi-joffre xinyi-joffre changed the title Add customDimensions using kwargs instead of args Add custom_dimensions using kwargs instead of args Jan 15, 2020
@xinyi-joffre
Copy link
Contributor Author

@lzchen this PR is ready to go, and passed the checks! Please let me know if you want me to fix anything else :). I can also help update the docs.microsoft.com entry for this once this is released.

@lzchen
Copy link
Contributor

lzchen commented Jan 15, 2020

@xinyi-joffre
Thanks for contributing. I can handle updating the docs, so no worries :)

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

@lzchen lzchen merged commit 22c1a28 into census-instrumentation:master Jan 15, 2020
@lzchen lzchen mentioned this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants