chore: allow tags for few metrics #39359
Conversation
WalkthroughThe changes update the metric filtering in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MeterFilter as NoTagsMeterFilter
Caller->>MeterFilter: map(Meter.Id)
alt Metric name starts with "appsmith"
MeterFilter->>MeterFilter: startsWithPrefix(metricName)
alt Metric matches exception prefix
MeterFilter-->>Caller: Return original Meter.Id
else Metric does not match exception prefix
MeterFilter-->>Caller: Return Meter.Id with tags removed
end
else
MeterFilter-->>Caller: Return original Meter.Id
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java (3)
10-11: Consider using constants for metric names.While the implementation is correct, consider defining metric names as constants to improve maintainability and prevent typos.
+ private static final String METRIC_PLUGIN_EXECUTION = "appsmith.total.plugin.execution"; + private static final String METRIC_SERVER_EXECUTION = "appsmith.total.server.execution"; + private static final String METRIC_DATASOURCE_CONTEXT = "appsmith.get.datasource.context"; - private static final List<String> seriesExceptionList = List.of( - "appsmith.total.plugin.execution", "appsmith.total.server.execution", "appsmith.get.datasource.context"); + private static final List<String> seriesExceptionList = List.of( + METRIC_PLUGIN_EXECUTION, METRIC_SERVER_EXECUTION, METRIC_DATASOURCE_CONTEXT);
16-16: Extract "appsmith" prefix as a constant.Consider extracting the "appsmith" prefix as a constant to improve maintainability.
+ private static final String METRIC_PREFIX = "appsmith"; - if (id.getName().startsWith("appsmith") && !startsWithAnyPrefix(id.getName())) { + if (id.getName().startsWith(METRIC_PREFIX) && !startsWithAnyPrefix(id.getName())) {
22-29: Consider using Java streams for better readability.The implementation is correct, but could be more concise using Java streams.
- private boolean startsWithAnyPrefix(String metricName) { - for (String prefix : seriesExceptionList) { - if (metricName.startsWith(prefix)) { - return true; - } - } - return false; - } + private boolean startsWithAnyPrefix(String metricName) { + return seriesExceptionList.stream() + .anyMatch(metricName::startsWith); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java (3)
10-11: Add documentation for exception list and consider externalization.Consider adding Javadoc explaining why these specific metrics need to retain their tags. For maintainability, these values could be moved to a configuration file.
14-20: Document method behavior and extract magic string.The implementation looks good, but consider:
- Adding Javadoc to explain the tag removal criteria
- Extracting "appsmith" as a constant
+ private static final String METRIC_PREFIX = "appsmith"; + + /** + * Removes tags from metrics that start with METRIC_PREFIX unless they're in the exception list. + * + * @param id The meter ID to process + * @return The processed meter ID + */ @Override public Meter.Id map(Meter.Id id) { - if (id.getName().startsWith("appsmith") && !startsWithPrefix(id.getName())) { + if (id.getName().startsWith(METRIC_PREFIX) && !startsWithPrefix(id.getName())) {
22-29: Consider using String.startsWith(prefix, 0) for potential performance optimization.The implementation is clean. For a minor optimization, you could use the String.startsWith overload that takes a starting position to avoid creating substrings:
private boolean startsWithPrefix(String metricName) { for (String prefix : seriesExceptionList) { - if (metricName.startsWith(prefix)) { + if (metricName.startsWith(prefix, 0)) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/NoTagsMeterFilter.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
ApekshaBhosale
added
ok-to-test
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13428563653
Commit: 494eede
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/ActionExecution/FrameworkFunctions_LocalStoreValueFunctions_spec.ts
- cypress/e2e/Regression/ClientSide/SetProperty/WidgetPropertySetters2_spec.ts
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_spec.ts
List of identified flaky tests.Thu, 20 Feb 2025 06:45:14 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit