Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

Add custom_dimensions using kwargs instead of args#837

Merged
lzchen merged 6 commits intocensus-instrumentation:masterfrom
xinyi-joffre:master
Jan 15, 2020
Merged

Add custom_dimensions using kwargs instead of args#837
lzchen merged 6 commits intocensus-instrumentation:masterfrom
xinyi-joffre:master

Conversation

@xinyi-joffre
Copy link
Copy Markdown
Contributor

@xinyi-joffre xinyi-joffre commented Jan 10, 2020 •
edited
Loading

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 xinyi-joffre requested a review from lzchen as a code owner January 10, 2020 16:22
@xinyi-joffre
Copy link
Copy Markdown
Contributor Author

xinyi-joffre commented Jan 10, 2020

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

lzchen
lzchen reviewed Jan 13, 2020
View reviewed changes
@xinyi-joffre
Copy link
Copy Markdown
Contributor Author

xinyi-joffre 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

lzchen
lzchen reviewed Jan 13, 2020
View reviewed changes
lzchen
lzchen reviewed Jan 13, 2020
View reviewed changes
lzchen
lzchen reviewed Jan 13, 2020
View reviewed changes
@lzchen
Copy link
Copy Markdown
Contributor

lzchen commented Jan 13, 2020 •
edited
Loading

@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
Copy Markdown
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
Copy Markdown
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.

xinyi-joffre
xinyi-joffre commented Jan 13, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

xinyi-joffre commented Jan 13, 2020 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

xinyi-joffre commented Jan 15, 2020

@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
Copy Markdown
Contributor

lzchen commented Jan 15, 2020

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

lzchen
lzchen approved these changes Jan 15, 2020
View reviewed changes
Copy link
Copy Markdown
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 Jan 15, 2020
Closed
@lzchen lzchen mentioned this pull request Feb 3, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@lzchen lzchen lzchen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xinyi-joffre @lzchen @googlebot