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.

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

Jump to bottom

Changes for adding default dimensions in CWSink. #209

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Changes for adding default dimensions in CWSink. #209

merged 1 commit into from
Jan 22, 2024

Conversation

vmmusings
Copy link
Member

@vmmusings vmmusings commented Dec 27, 2023 •
edited
Loading

Description

  • Added FlintMetricSource, which will be used in upcoming PRs for publish all kinds of metrics.
  • Added default dimensions of domainId, jobId, applicationId while publishing metrics in CloudWatch Sink.
  • Removed prefixes appended to metricNames. For eg:
    Previous Metric Name:
    driver.00fg29jd50i99g0m.LiveListenerBus.queue.appStatus.listenerProcessingTime
    Current Metric Name:
    LiveListenerBus.queue.appStatus.listenerProcessingTime
  • Changed the separator in DimensionedName encoder to "##" to remove conflicts with names containing ": "

Sample Metrics Testing:
Screenshot 2024-01-08 at 8 10 32 AM

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@vmmusings vmmusings force-pushed the main branch 3 times, most recently from 6d6cc15 to c13af49 Compare January 5, 2024 19:25
@noCharger
Copy link
Collaborator

@vamsi-amazon Would you prefer a seperate commit for AOP or should I contribute to this PR?

@vmmusings
Copy link
Member Author

@vamsi-amazon Would you prefer a seperate commit for AOP or should I contribute to this PR?

Separate PR. I would minimize the changes in this PR and would limit to just publishing opensearch response code metrics. You can refactor and add AOP. Will publish the PR by end of the day.

@vmmusings vmmusings force-pushed the main branch 4 times, most recently from eca7d80 to 791220c Compare January 8, 2024 16:03
@vmmusings vmmusings marked this pull request as ready for review January 8, 2024 16:11
@vmmusings vmmusings changed the title Add flint opensearch metrics Changes for adding default dimensions in CWSink. Jan 8, 2024
penghuo
penghuo reviewed Jan 8, 2024
View reviewed changes
.../src/main/java/org/opensearch/flint/core/metrics/reporter/DimensionedCloudWatchReporter.java
.withDimensions(dimensions)
.withUnit(standardUnit));
}
}

@NotNull
private Pair<String, Set<Dimension>> getFinalMetricNameAndDefaultDimensions(DimensionedName dimensionedName) {
final String jobId = System.getenv().getOrDefault("SERVERLESS_EMR_JOB_ID", UNKNOWN);
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the cons to add more dimensions in CW?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vamsi-amazon @penghuo can we assume "SERVERLESS_EMR_JOB_ID" is isolated at job level?

Copy link
Member Author

Choose a reason for hiding this comment

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

@penghuo Didn't get your question?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add more dimensions in the DimensionedName object while publishing the metric.

noCharger
noCharger reviewed Jan 9, 2024
View reviewed changes
.../src/main/java/org/opensearch/flint/core/metrics/reporter/DimensionedCloudWatchReporter.java
private Pair<String, Set<Dimension>> getFinalMetricNameAndDefaultDimensions(DimensionedName dimensionedName) {
final String jobId = System.getenv().getOrDefault("SERVERLESS_EMR_JOB_ID", UNKNOWN);
final String applicationId = System.getenv().getOrDefault("SERVERLESS_EMR_VIRTUAL_CLUSTER_ID", UNKNOWN);
final String domainId = System.getenv().getOrDefault("FLINT_CLUSTER_NAME", UNKNOWN);
Copy link
Collaborator

@noCharger noCharger Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also shall we include this in skip index mapping? https://github.com/opensearch-project/sql/blob/efb159a8cb0560fbde996cdf7c72ce82deb15681/spark/src/test/resources/flint-index-mappings/flint_mys3_default_http_logs_skipping_index.json#L20

Copy link
Member Author

Choose a reason for hiding this comment

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

@noCharger Didn't get you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vamsi-amazon do we persist cluster name in index meta field?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. I guess no. What are you trying to get to?

@vmmusings vmmusings force-pushed the main branch 5 times, most recently from fa73a34 to 7f40f4d Compare January 16, 2024 20:46
@vmmusings
Add flint opensearch metrics
aafae22 
Signed-off-by: Vamsi Manohar <[email protected]>
@dai-chen dai-chen requested a review from penghuo January 19, 2024 18:14
@dai-chen dai-chen added the 0.2 label Jan 19, 2024
@noCharger
Copy link
Collaborator

This fix the backport issue #230

noCharger
noCharger reviewed Jan 21, 2024
View reviewed changes
.../src/main/java/org/opensearch/flint/core/metrics/reporter/DimensionedCloudWatchReporter.java
Comment on lines +88 to +94
public static final String DIMENSION_JOB_ID = "jobId";

public static final String DIMENSION_APPLICATION_ID = "applicationId";

public static final String DIMENSION_DOMAIN_ID = "domainId";

public static final String DIMENSION_INSTANCE_ROLE = "instanceRole";
Copy link
Collaborator

Choose a reason for hiding this comment

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

although it's good to provide insights, DIMENSION_JOB_ID / DIMENSION_APPLICATION_ID / DIMENSION_DOMAIN_ID / DIMENSION_INSTANCE_ROLE are actually high cardinality dimensions which considered as seperate metrics. It will bring extra cost setting them as default dimensions. Ref: https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch_billing.html

@vmmusings vmmusings merged commit 6dc47de into opensearch-project:main Jan 22, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noCharger noCharger noCharger approved these changes

@dai-chen dai-chen dai-chen approved these changes

@rupal-bq rupal-bq Awaiting requested review from rupal-bq

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@kaituo kaituo Awaiting requested review from kaituo kaituo is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB YANG-DB is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

Assignees
No one assigned
Labels
0.2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vmmusings @noCharger @penghuo @dai-chen