-
Notifications
You must be signed in to change notification settings - Fork 33
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 OpenSearch metrics #229
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noCharger
requested review from
dai-chen,
rupal-bq,
vmmusings,
penghuo,
anirudha,
kaituo and
YANG-DB
as code owners
January 19, 2024 17:57
penghuo
reviewed
Jan 20, 2024
flint-core/src/main/scala/org/opensearch/flint/core/metrics/RestHighLevelClientWrapper.java
Outdated
Show resolved
Hide resolved
noCharger
force-pushed
the
flint-os-metrics
branch
from
January 25, 2024 22:53
300501e
to
cfaf065
Compare
noCharger
force-pushed
the
flint-os-metrics
branch
from
January 25, 2024 23:23
cfaf065
to
3df3a21
Compare
noCharger
force-pushed
the
flint-os-metrics
branch
4 times, most recently
from
January 26, 2024 19:17
d6290fb
to
284669d
Compare
Signed-off-by: Louis Chu <[email protected]>
noCharger
force-pushed
the
flint-os-metrics
branch
from
January 26, 2024 19:29
284669d
to
b47386e
Compare
dai-chen
reviewed
Jan 26, 2024
flint-core/src/main/scala/org/opensearch/flint/core/FlintClient.java
Outdated
Show resolved
Hide resolved
spark-sql-application/src/main/scala/org/apache/spark/sql/FlintREPL.scala
Show resolved
Hide resolved
Signed-off-by: Louis Chu <[email protected]>
vmmusings
reviewed
Jan 26, 2024
flint-core/src/main/java/org/opensearch/flint/core/IRestHighLevelClient.java
Outdated
Show resolved
Hide resolved
vmmusings
reviewed
Jan 26, 2024
flint-core/src/main/java/org/opensearch/flint/core/metrics/MetricsUtil.java
Outdated
Show resolved
Hide resolved
vmmusings
reviewed
Jan 26, 2024
flint-core/src/main/java/org/opensearch/flint/core/metrics/MetricsUtil.java
Outdated
Show resolved
Hide resolved
vmmusings
reviewed
Jan 26, 2024
flint-core/src/main/java/org/opensearch/flint/core/RestHighLevelClientWrapper.java
Show resolved
Hide resolved
vmmusings
reviewed
Jan 26, 2024
flint-core/src/main/java/org/opensearch/flint/core/RestHighLevelClientWrapper.java
Show resolved
Hide resolved
vmmusings
reviewed
Jan 26, 2024
.../src/main/java/org/opensearch/flint/core/metrics/reporter/DimensionedCloudWatchReporter.java
Show resolved
Hide resolved
vmmusings
reviewed
Jan 27, 2024
.../src/main/java/org/opensearch/flint/core/metrics/reporter/DimensionedCloudWatchReporter.java
Outdated
Show resolved
Hide resolved
vmmusings
reviewed
Jan 27, 2024
flint-core/src/main/java/org/opensearch/flint/core/metrics/MetricsUtil.java
Outdated
Show resolved
Hide resolved
Thanks for the changes. |
Signed-off-by: Louis Chu <[email protected]>
penghuo
reviewed
Jan 29, 2024
flint-core/src/main/java/org/opensearch/flint/core/metrics/MetricsUtil.java
Show resolved
Hide resolved
flint-core/src/main/java/org/opensearch/flint/core/metrics/MetricsUtil.java
Show resolved
Hide resolved
flint-core/src/main/java/org/opensearch/flint/core/RestHighLevelClientWrapper.java
Show resolved
Hide resolved
flint-core/src/main/java/org/opensearch/flint/core/RestHighLevelClientWrapper.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Louis Chu <[email protected]>
noCharger
force-pushed
the
flint-os-metrics
branch
from
January 29, 2024 22:37
a97dc80
to
5775a0a
Compare
penghuo
approved these changes
Jan 31, 2024
dai-chen
approved these changes
Jan 31, 2024
vmmusings
approved these changes
Feb 1, 2024
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/opensearch-spark/backport-0.1 0.1
# Navigate to the new working tree
pushd ../.worktrees/opensearch-spark/backport-0.1
# Create a new branch
git switch --create backport/backport-229-to-0.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6f708847c5b91d5d702b5d37d7afc8edc6c13e15
# Push it to GitHub
git push --set-upstream origin backport/backport-229-to-0.1
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/opensearch-spark/backport-0.1 Then, create a pull request where the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR is to
Metrics included in this PR:
Approaches
There are three approaches to achieving aspect-oriented programming (AOP) to provide metrics within Flint: AspectJ library, dynamic proxies, and wrapper (decorator).
AspectJ
Java Dynamic Proxies
AOP Implementation: Dynamic proxies make it easier to implement AOP by intercepting methods via reflection. They are limited to interfaces and cannot directly change class behavior or attributes.
Code example:
Performance: Because of the usage of reflection, they have a larger overhead than AspectJ approach. However, for many common applications, this expense is insignificant.
Use cases: Suitable for applications in which the AOP requirements are confined to method interception and interfaces are the norm. It's a suitable alternative for basic AOP scenarios and projects where introducing a dependency like AspectJ isn't ideal.
Wrapper (Decorator) Approach implemented in this PR
AOP Implementation: This approach doesn't use AOP in the traditional sense but can mimic some aspects of it, like method interception, by manually wrapping classes. It's more of a design pattern than an AOP solution and requires explicit coding for each method and class.
Performance: The wrapper approach introduces additional method calls, but these are usually direct and thus have less overhead than dynamic proxies. The performance impact is generally minimal, but it's more than AspectJ.
Use Cases: Best for situations where you need to wrap a limited number of classes or methods, and the interface of the classes is stable. It's straightforward and easy to understand, making it a good choice for smaller projects or when simplicity is key.
Overall Comparison
Performance Hierarchy: AspectJ > Wrapper Approach > Dynamic Proxies. AspectJ is typically the fastest due to direct bytecode manipulation, followed by the wrapper approach with its direct method calls, and finally dynamic proxies with the overhead of reflection.
Complexity and Flexibility: AspectJ is the most complex but also the most flexible and powerful. Dynamic proxies offer a balance between simplicity and flexibility, suitable for medium complexity needs. The wrapper approach is the simplest but the least flexible, especially for large-scale or changing codebases.
Suitability: AspectJ is ideal for large, complex applications with extensive cross-cutting concerns. Dynamic proxies are good for medium-scale applications or where interface-based programming is predominant. The wrapper approach is best for smaller applications or when you need a straightforward solution without extra dependencies or complexities.
Issues Resolved
#220
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.