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

Live Metrics Filtering Part 6: Error Tracker #43744

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

harsimar
Copy link
Member

@harsimar harsimar commented Jan 9, 2025

Description

When parsing a new filtering configuration, this PR will also track configuration errors. Configuration errors are logged to applicationinsights.log at every config change. In addition, configuration errors are sent as a part of the post request body for as long as the faulty configuration is being used.

There is also some verbose level logging added for debug purposes.
Manual testing was done in addition to some modifications to existing tests.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@harsimar harsimar requested a review from trask as a code owner January 9, 2025 01:19
@github-actions github-actions bot added the OpenTelemetry OpenTelemetry instrumentation label Jan 9, 2025
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

Copy link
Contributor

@jeanbisutti jeanbisutti left a comment

Choose a reason for hiding this comment

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

It seems that the Validator class has 2 different responsabilities:

Classes with one responsability are easier to understand and maintain.

import com.azure.monitor.opentelemetry.autoconfigure.implementation.quickpulse.swagger.models.FilterInfo;

import java.util.HashMap;
import java.util.Map;

public class CustomDimensions {
private final Map<String, String> customDimensions;
private static ClientLogger logger = new ClientLogger(CustomDimensions.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static ClientLogger logger = new ClientLogger(CustomDimensions.class);
private static final ClientLogger logger = new ClientLogger(CustomDimensions.class);

@@ -47,9 +49,13 @@ public double getCustomDimValueForProjection(String key) {
try {
return Double.parseDouble(value);
} catch (NumberFormatException e) {

logger.verbose(
"The value for the custom dimension could not be converted to a numeric value for a derived metric projection");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"The value for the custom dimension could not be converted to a numeric value for a derived metric projection");
value + " for the custom dimension could not be converted to a numeric value for a derived metric projection");

@@ -15,17 +16,26 @@ public class DependencyDataColumns implements TelemetryColumns {
private final CustomDimensions customDims;
private final Map<String, Object> mapping = new HashMap<>();

private static ClientLogger logger = new ClientLogger(DependencyDataColumns.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static ClientLogger logger = new ClientLogger(DependencyDataColumns.class);
private static final ClientLogger logger = new ClientLogger(DependencyDataColumns.class);

mapping.put(KnownDependencyColumns.SUCCESS, rdData.isSuccess());
mapping.put(KnownDependencyColumns.NAME, rdData.getName());
int resultCode;
try {
resultCode = Integer.parseInt(rdData.getResultCode());
} catch (NumberFormatException e) {
logger.verbose("The provided result code could not be converted to a numeric value: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.verbose("The provided result code could not be converted to a numeric value: {}",
logger.verbose(rdData.getResultCode() + " result code could not be converted to a numeric value: {}",


long durationMicroSec = FormattedDuration.getDurationFromTelemetryItemDurationString(requestData.getDuration());
if (durationMicroSec == -1) {
logger.verbose("The provided timestamp could not be converted to microseconds: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.verbose("The provided timestamp could not be converted to microseconds: {}",
logger.verbose(requestData.getDuration() + " provided timestamp could not be converted to microseconds: {}",

mapping.put(KnownRequestColumns.NAME, requestData.getName());
int responseCode;
try {
responseCode = Integer.parseInt(requestData.getResponseCode());
} catch (NumberFormatException e) {
responseCode = -1;
logger.verbose("The provided response code could not be converted to a numeric value: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.verbose("The provided response code could not be converted to a numeric value: {}",
logger.verbose(requestData.getResponseCode() + " provided response code could not be converted to a numeric value: {}",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OpenTelemetry OpenTelemetry instrumentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants