Skip to content

Configuration arguments validation #258

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.

Sign up for GitHub

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

Closed
jeremydvoss wants to merge 3 commits into from
Closed

Configuration arguments validation #258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c545bf2
Add type validation for configuration arguments
jeremydvoss Mar 14, 2023
e0cd36e
lint
jeremydvoss Mar 17, 2023
6e0cc4a
Merge branch 'main' into type-validation
lzchen Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
([#259](https://github.com/microsoft/ApplicationInsights-Python/pull/259))
- Change interval params to use `_ms` as suffix
([#260](https://github.com/microsoft/ApplicationInsights-Python/pull/260))
- Add type validation for configuration arguments
([#258](https://github.com/microsoft/ApplicationInsights-Python/pull/258))

## [1.0.0b10](https://github.com/microsoft/ApplicationInsights-Python/releases/tag/v1.0.0b10) - 2023-02-23

Expand Down
39 changes: 37 additions & 2 deletions azure-monitor-opentelemetry/azure/monitor/opentelemetry/_configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
# license information.
# --------------------------------------------------------------------------
from logging import NOTSET, getLogger
from typing import Any, Dict
from typing import Any, Dict, Sequence

from azure.monitor.opentelemetry._types import ConfigurationValue
from azure.monitor.opentelemetry.exporter import (
Expand Down Expand Up @@ -53,7 +53,7 @@ def configure_azure_monitor(**kwargs) -> None:
end user to configure OpenTelemetry and Azure monitor components. The
configuration can be done via arguments passed to this function.
:keyword str connection_string: Connection string for your Application Insights resource.
:keyword Sequence[str] connection_string: Specifies the libraries with instrumentations to be enabled.
:keyword Sequence[str] exclude_instrumentations: Specifies instrumentations you do not want to enable.
:keyword Resource resource: Specified the OpenTelemetry [resource][opentelemetry_spec_resource] associated with your application.
:keyword bool disable_logging: If set to `True`, disables collection and export of logging telemetry. Defaults to `False`.
:keyword bool disable_metrics: If set to `True`, disables collection and export of metric telemetry. Defaults to `False`.
Expand All @@ -74,6 +74,7 @@ def configure_azure_monitor(**kwargs) -> None:
"""

configurations = _get_configurations(**kwargs)
_validate_configurations(configurations)

disable_tracing = configurations.get("disable_tracing", False)
disable_logging = configurations.get("disable_logging", False)
Expand Down Expand Up @@ -199,3 +200,37 @@ def _setup_instrumentations(configurations: Dict[str, ConfigurationValue]):
lib_name,
exc_info=ex,
)


def _is_instance_or_none(var, type):
return isinstance(var, type) or var is None


def _validate_configurations(configurations):
assert _is_instance_or_none(configurations.get("connection_string"), str)
assert _is_instance_or_none(
configurations.get("exclude_instrumentations"), Sequence
)
assert _is_instance_or_none(configurations.get("resource"), Resource)
Copy link
Copy Markdown
Contributor

@lzchen lzchen Mar 20, 2023

Choose a reason for hiding this comment

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

The idea of this PR is not really to do type checking (it would be too cumbersome to always check for typing, plus this is why we use typing.

We want to only have a few validation checks for logic (like interval needs to be > 0).

The idea for Python and for the distro is that if input is not the correct type, it will not do anything with that input.

assert _is_instance_or_none(configurations.get("disable_logging"), bool)
assert _is_instance_or_none(configurations.get("disable_metrics"), bool)
assert _is_instance_or_none(configurations.get("disable_tracing"), bool)
assert _is_instance_or_none(configurations.get("logging_level"), int)
assert _is_instance_or_none(configurations.get("logger_name"), str)
assert _is_instance_or_none(
configurations.get("logging_export_interval_millis"), int
Copy link
Copy Markdown
Contributor

@lzchen lzchen Mar 20, 2023

Choose a reason for hiding this comment

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

Non-negative values for intervals.

)
assert _is_instance_or_none(configurations.get("metric_readers"), Sequence)
assert _is_instance_or_none(configurations.get("views"), Sequence)
assert _is_instance_or_none(configurations.get("sampling_ratio"), float)
assert _is_instance_or_none(
configurations.get("tracing_export_interval_millis"), int
)
for library in _SUPPORTED_INSTRUMENTED_LIBRARIES:
assert _is_instance_or_none(
configurations.get(library + "_config"), Dict
Copy link
Copy Markdown
Contributor

@lzchen lzchen Mar 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Might have to rebase this and validate instrumentation_config

)
assert _is_instance_or_none(
configurations.get("disable_offline_storage"), bool
)
assert _is_instance_or_none(configurations.get("storage_directory"), str)
15 changes: 9 additions & 6 deletions azure-monitor-opentelemetry/tests/configuration/test_configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

import unittest
from logging import WARN
from unittest.mock import Mock, patch

from azure.monitor.opentelemetry._configure import (
Expand All @@ -25,6 +26,8 @@
configure_azure_monitor,
)

TEST_LOGGING_LEVEL = WARN


class TestConfigure(unittest.TestCase):
@patch(
Expand Down Expand Up @@ -56,7 +59,7 @@ def test_configure_azure_monitor(
"disable_logging": False,
"disable_metrics": False,
"logging_export_interval_ms": 10000,
"logging_level": "test_logging_level",
"logging_level": TEST_LOGGING_LEVEL,
"logger_name": "test_logger_name",
"metric_readers": "test_metric_readers",
"service_name": "test_service_name",
Expand Down Expand Up @@ -104,7 +107,7 @@ def test_configure_azure_monitor_disable_tracing(
"disable_logging": False,
"disable_metrics": False,
"logging_export_interval_ms": 10000,
"logging_level": "test_logging_level",
"logging_level": TEST_LOGGING_LEVEL,
"logger_name": "test_logger_name",
"service_name": "test_service_name",
"service_namespace": "test_namespace",
Expand Down Expand Up @@ -151,7 +154,7 @@ def test_configure_azure_monitor_disable_logging(
"disable_logging": True,
"disable_metrics": False,
"logging_export_interval_ms": 10000,
"logging_level": "test_logging_level",
"logging_level": TEST_LOGGING_LEVEL,
"logger_name": "test_logger_name",
"service_name": "test_service_name",
"service_namespace": "test_namespace",
Expand Down Expand Up @@ -198,7 +201,7 @@ def test_configure_azure_monitor_disable_metrics(
"disable_logging": False,
"disable_metrics": True,
"logging_export_interval_ms": 10000,
"logging_level": "test_logging_level",
"logging_level": TEST_LOGGING_LEVEL,
"service_name": "test_service_name",
"service_namespace": "test_namespace",
"service_instance_id": "test_id",
Expand Down Expand Up @@ -337,7 +340,7 @@ def test_setup_logging(
"connection_string": "test_cs",
"disable_logging": False,
"logging_export_interval_ms": 10000,
"logging_level": "test_logging_level",
"logging_level": TEST_LOGGING_LEVEL,
"logger_name": "test_logger_name",
}
_setup_logging(resource_mock, configurations)
Expand All @@ -353,7 +356,7 @@ def test_setup_logging(
blrp_init_mock
)
logging_handler_mock.assert_called_once_with(
level="test_logging_level", logger_provider=lp_init_mock
level=TEST_LOGGING_LEVEL, logger_provider=lp_init_mock
)
get_logger_mock.assert_called_once_with("test_logger_name")
logger_mock.addHandler.assert_called_once_with(
Expand Down