Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 40 additions & 0 deletions .claude/skills/gh-pull-request/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,46 @@ license-eye header check

If invalid files are found, fix with `license-eye header fix` and re-check.

### 3. Unnecessary fully-qualified class names

The project checkstyle forbids inline FQCNs — every type reference in code should resolve
through an `import`, not a fully-qualified name. Checkstyle does not always catch this (it
misses cases like inline `java.util.HashMap`, `java.util.concurrent.TimeUnit`, or
`org.apache.skywalking.oap.server.telemetry.api.HistogramMetrics.Timer` used as a local
variable type, generic parameter, or `new` target). Audit the files the branch touched
before pushing:

Use the `Grep` tool (ripgrep) rather than BSD `grep` on macOS — the scan below relies on a
negative lookahead that BSD `grep` doesn't support and GNU `grep -P` does:

```
pattern: ^(?!\s*(import |package |\s*\*)).*\b(java\.util\.|java\.io\.|java\.nio\.|java\.util\.concurrent\.|javassist\.|org\.apache\.skywalking\.)[A-Z][A-Za-z0-9_]*
glob: *.java
output_mode: content
-n: true
```

Scope the scan to files the branch touched, not the whole tree — pre-existing FQDNs on
unrelated files generate noise. Use `git diff --name-only master...HEAD -- '*.java'` to get
the changed list, then run the ripgrep pattern against each.

Acceptable exceptions (same as the `CLAUDE.md` rule):
- Two classes with the same simple name would collide if both imported.
- A Javadoc `{@link}` where the short name would be ambiguous to the reader.
- Inside a string literal (e.g., a class name passed to `Class.forName`).

Fix every other hit — add an `import` and switch to the short name. This includes
`new java.util.HashMap<>()`, `java.util.Set<String>` parameter types, and
`org.apache.skywalking.oap.server.telemetry.api.HistogramMetrics.Timer` as a local
variable type. Field declarations, method signatures, local variables, and generic
type arguments should all use the imported short name.

Re-run checkstyle after the fix — a sloppy `sed`/`replace_all` can corrupt the `import`
line itself (e.g., turning `import java.util.concurrent.locks.ReentrantLock;` into
`import ReentrantLock;`), which causes a cryptic checkstyle `Range [0, -1) out of
bounds for length N` error, not a normal violation line. If you see that error, inspect
the imports block first.

## Commit and push

After checks pass, commit and push:
Expand Down
16 changes: 15 additions & 1 deletion .github/workflows/skywalking.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,9 @@ jobs:
distribution: temurin
- name: Integration test
run: |
# Exclude slow integration tests and run those tests separately below.
# Exclude slow integration tests (run in slow-integration-test). Runtime-rule
# and BanyanDB storage CRUD are verified end-to-end in the dedicated e2e cases
# (see test/e2e-v2/cases/runtime-rule/ and test/e2e-v2/cases/banyandb).
./mvnw -B clean integration-test -Dcheckstyle.skip -DskipUTs=true -DexcludedGroups=slow || \
./mvnw -B clean integration-test -Dcheckstyle.skip -DskipUTs=true -DexcludedGroups=slow

Expand Down Expand Up @@ -394,6 +396,18 @@ jobs:
config: test/e2e-v2/cases/storage/es/es-sharding/e2e.yaml
env: ES_VERSION=8.18.8

- name: Runtime Rule MAL Storage BanyanDB
config: test/e2e-v2/cases/runtime-rule/mal-storage/banyandb/e2e.yaml
- name: Runtime Rule MAL Storage PostgreSQL
config: test/e2e-v2/cases/runtime-rule/mal-storage/postgresql/e2e.yaml
- name: Runtime Rule MAL Storage Elasticsearch 8.18.8
config: test/e2e-v2/cases/runtime-rule/mal-storage/elasticsearch/e2e.yaml
env: ES_VERSION=8.18.8
- name: Runtime Rule LAL Hot-Update
config: test/e2e-v2/cases/runtime-rule/lal/e2e.yaml
- name: Runtime Rule Cluster Convergence
config: test/e2e-v2/cases/runtime-rule/cluster/e2e.yaml

- name: Alarm ES
config: test/e2e-v2/cases/alarm/es/e2e.yaml
- name: Alarm ES Sharding
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,5 @@ test/script-cases/scripts/**/*.generated-classes/

# Claude Code local settings
.claude/settings.local.json
.claude/scheduled_tasks.lock
*.generated-classes/
4 changes: 2 additions & 2 deletions .licenserc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,10 @@ dependency:
version: 1.12.0
license: Apache-2.0
- name: build.buf.protoc-gen-validate:pgv-java-stub
version: 1.2.1
version: 1.3.0
license: Apache-2.0
- name: build.buf.protoc-gen-validate:protoc-gen-validate
version: 1.2.1
version: 1.3.0
license: Apache-2.0
- name: com.aayushatharva.brotli4j:service
version: 1.20.0
Expand Down
22 changes: 22 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,17 @@ public class XxxModuleProvider extends ModuleProvider {
- No star imports (`import xxx.*`)
- No unused or redundant imports
- No empty statements (standalone `;`)
- No fully-qualified class names inline in code — always add an `import` statement and
use the short name. Acceptable exceptions: (a) two classes with the same simple name
would collide if both imported, (b) the class appears exactly once in a Javadoc
`{@link}` where the short name would be ambiguous to the reader. Field declarations,
method signatures, local variables, and generic type arguments should always use the
imported short name — `private RemoteClientManager rcm;`, not `private
org.apache.skywalking.oap.server.core.remote.client.RemoteClientManager rcm;`.
- No one-line delegate methods. A wrapper whose only body is a single forwarding call
to another class (`return Other.foo(a, b);`) adds a hop without value. Inline the
call at the use site, or call the underlying object directly (including via method
reference: `obj::foo` instead of `this::wrapperOfFoo`).

**Required patterns:**
- `@Override` annotation required for overridden methods
Expand All @@ -105,6 +116,13 @@ public class XxxModuleProvider extends ModuleProvider {
- Package names: `org.apache.skywalking.*` or `test.apache.skywalking.*`
- Type names: `PascalCase` or `UPPER_CASE_WITH_UNDERSCORES`
- Local variables/parameters/members: `camelCase`
- **Function-oriented naming, not abstract metaphor**: classes and methods are named for
what they do, not for an abstract concept. Prefer concrete verbs (`load`, `apply`,
`unregister`, `compile`, `verify`, `commit`, `rollback`) over metaphorical ones
(`seed`, `hydrate`, `bootstrap`, `prime`). Class names follow the same rule —
`StaticRuleLoader` (loads static rules), not `StaticBundleSeeder`; `DSLSyncTimer` (syncs
DB → state on a timer), not `TickRunner`. If you can't name a method without reaching
for a metaphor, the method is probably doing too much; split it.

**File limits:**
- Max file length: 3000 lines
Expand Down Expand Up @@ -257,6 +275,10 @@ Actions owned by `actions/*` (GitHub), `github/*`, and `apache/*` are always all
10. **Relative paths in docs are valid**: Relative file paths (e.g., `../../../oap-server/...`) in documentation work both in the repo and on the documentation website, supported by website build tooling
11. **Module service registration**: When adding a service to `CoreModule.services()`, update ALL `CoreModuleProvider` implementations — not just the main one. Search with `grep -rn "extends CoreModuleProvider" oap-server/ --include="*.java"`. The `MockCoreModuleProvider` in `server-tools/profile-exporter/` also needs it, or the profile exporter e2e test will fail at startup.
12. **Multiple OAP packagings**: The OAP server is not only the main `server-starter`. The `server-tools/` directory contains standalone tools (e.g., profile exporter) that boot with mock module providers and a subset of modules. Changes to core module contracts (services, required modules) must be reflected in these tools too.
13. **`moduleManager.find(X.NAME)` requires `X.NAME` in `requiredModules()`**: every call to `moduleManager.find(SomeModule.NAME)` (direct or through a helper) must have `SomeModule.NAME` in the provider's `requiredModules()` array. Missing declarations cause runtime exceptions the first time the code path fires — not at module boot. Wrapping the call in `try { ... } catch (Throwable)` is NOT a substitute; declare the module and keep the try/catch only for defensive handling of transient provider outages. When auditing a branch, grep for `moduleManager.find(` across the touched module and verify each target name appears in `requiredModules()`. Example modules that frequently catch teams out: `AlarmModule` (used by alarm-kernel reset), `LogAnalyzerModule` (used by LAL factory lookup).
14. **Don't look up `ClusterModule` services directly**: the `ClusterModule` (ZooKeeper / K8s / Nacos coordination) exposes `ClusterRegister` / `ClusterNodesQuery` / `ClusterCoordinator`. Most receiver / analyzer modules don't declare `ClusterModule` in `requiredModules()`, so calling `moduleManager.find(ClusterModule.NAME)` will throw at runtime. Instead, go through `CoreModule`'s `RemoteClientManager` service — it's already populated by the cluster module and exposes the peer list every OAP needs. If a module genuinely needs cluster-coordinator primitives, declare `ClusterModule.NAME` in `requiredModules()` explicitly.
15. **No `ThreadLocal` side-channels to hijack downstream behaviour**: routing a caller's intent through a `ThreadLocal` that downstream code reads (e.g., `if (PeerMode.isActive()) skipSomething()`) is almost always the wrong answer — it creates invisible coupling between far-apart code paths, leaks across async hand-offs (executors, gRPC threads, Armeria event loops), and makes the behaviour impossible to understand from a method signature. The correct fix is almost always to **extend the interface** — add a parameter, a new method, a new mode enum that appears in the signature. Rare exceptions: propagating OpenTelemetry context where the whole industry has standardised on `ThreadLocal`, or security principals enforced by a framework. In all other cases, prefer an explicit API extension, even if it costs more lines.
16. **BanyanDB schema-visibility: fence on `mod_revision`, do NOT poll metadata**: every BanyanDB Create / Update / Delete returns an etcd `mod_revision` (0 on a delete that didn't record a tombstone). After firing DDL, fence on `BanyanDBClient.getSchemaWatcher().awaitRevisionApplied(maxRev, timeout)` before unparking dispatch / firing data writes — this blocks until every data node has caught up, which the registry's read-after-write does not guarantee. For deletes that returned `mod_revision == 0`, fall back to `awaitSchemaDeleted(SchemaKey, timeout)`. The previous "poll `findMeasure` until you can read your own write" idiom existed before the `SchemaBarrierService` proto landed and has been replaced — do not reintroduce it. JDBC and ES are synchronous-DDL on the coordinator so they don't need a fence.

## Analysis and Design Principles

Expand Down
4 changes: 2 additions & 2 deletions apm-protocol/apm-network/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,11 @@
protobuf-java version that grpc depends on.
-->
<protocArtifact>
com.google.protobuf:protoc:${com.google.protobuf.protoc.version}:exe:${os.detected.classifier}
com.google.protobuf:protoc:${protobuf-java.version}:exe:${os.detected.classifier}
</protocArtifact>
<pluginId>grpc-java</pluginId>
<pluginArtifact>
io.grpc:protoc-gen-grpc-java:${protoc-gen-grpc-java.plugin.version}:exe:${os.detected.classifier}
io.grpc:protoc-gen-grpc-java:${grpc.version}:exe:${os.detected.classifier}
</pluginArtifact>
</configuration>
<executions>
Expand Down
Loading
Loading