Skip to content

"Applying" potential Copilot code review rules #2182

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

"Applying" potential Copilot code review rules #2182

wants to merge 6 commits into from

Conversation

trask
Copy link
Member

@trask trask commented Aug 25, 2025

Hopefully this is reflective of the changes copilot code review will suggest after #2170.

Please comment either here or there if you have suggestions about the style-guide.md that was used to generate this PR.

This PR was generated by running this prompt module-by-module:

Make minimal changes to update {module} module to follow all of the recommendations in docs/style-guide.md. Ignore the @Nullable annotation usage section of the style guide.

I made very minor manual fixes to Copilot's work after the fact, including reverting adding final to public classes in the stable aws-xray module.

(I skipped the consistent-sampling module for now, will apply to that later)

@trask trask requested a review from a team as a code owner August 25, 2025 03:36
@trask trask changed the title Copilot code review test "Applying" potential Copilot code review rules Aug 25, 2025
KarstenSchnitter
KarstenSchnitter reviewed Aug 26, 2025
View reviewed changes
Copy link
Contributor

@KarstenSchnitter KarstenSchnitter left a comment

Choose a reason for hiding this comment

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

I reviewed the changes to the cloudfoundry-resources subproject. They look fine, I approve.

SylvainJuge
SylvainJuge approved these changes Aug 26, 2025
View reviewed changes
Copy link
Contributor

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

LGTM for the following modules:

  • inferred-spans
  • jmx-metrics
  • jmx-scraper
  • span-stacktrace

@trask trask force-pushed the copilot-code-review-test branch from ce7b79f to 2ae0ed8 Compare August 26, 2025 18:56
breedx-splk
breedx-splk reviewed Aug 29, 2025
View reviewed changes
aws-resources/build.gradle.kts Outdated
@@ -23,6 +26,7 @@ dependencies {
testImplementation("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure")
testImplementation("io.opentelemetry:opentelemetry-api-incubator")
testImplementation("io.opentelemetry:opentelemetry-sdk-testing")
testImplementation("com.google.auto.service:auto-service")
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems harmless but I don't immediately see why it's required.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, removed

Copy link
Member Author

Choose a reason for hiding this comment

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

I found out this is required, because javac -Werror on the test files throws an error if it can't find the annotations used on the classes that the tests reference 🤯

Which I assume affects downstream consumer of our libraries also as long as we make autoservice a compileOnly dependency.

so I reverted all the autoservice changes in this PR so we can discuss it separately

breedx-splk
breedx-splk reviewed Aug 29, 2025
View reviewed changes
azure-resources/build.gradle.kts
Comment on lines -8 to +9
description = "OpenTelemetry GCP Resources Support"
otelJava.moduleName.set("io.opentelemetry.contrib.gcp.resource")
description = "OpenTelemetry Azure Resources Support"
otelJava.moduleName.set("io.opentelemetry.contrib.azure.resource")
Copy link
Contributor

Choose a reason for hiding this comment

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

🤯 🥳

breedx-splk
breedx-splk reviewed Aug 29, 2025
View reviewed changes
breedx-splk
breedx-splk reviewed Aug 29, 2025
View reviewed changes
...pans/src/main/java/io/opentelemetry/contrib/inferredspans/InferredSpansProcessorBuilder.java
Comment on lines -30 to +31
Arrays.asList(
asList(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, this is fine I guess, but this one seems less idiomatic to me.

breedx-splk
breedx-splk reviewed Aug 29, 2025
View reviewed changes
processors/src/main/java/io/opentelemetry/contrib/filter/FilteringLogRecordProcessor.java Outdated
Comment on lines 16 to 17
public final LogRecordProcessor delegate;
public final Predicate<LogRecordData> predicate;
private final LogRecordProcessor delegate;
private final Predicate<LogRecordData> predicate;
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this, but note that it is potentially breaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, good point, I decided to revert all the breaking changes (to alpha modules) to follow-up PRs so they get the visibility they deserve

breedx-splk
breedx-splk reviewed Aug 29, 2025
View reviewed changes
breedx-splk
breedx-splk reviewed Aug 29, 2025
View reviewed changes
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

So @trask and I talked a little bit about this out of band. I think it's good, but we agreed that the classes in "internal" packages (and also package-private) should retain their final designations (which this PR attempts to remove).

The final is good because it also discourages other developers, even within the project itself, from introducing inheritance, and it maintains the status quo.

Otherwise, looks pretty good.

@trask trask force-pushed the copilot-code-review-test branch from 3049362 to 3807cfd Compare August 30, 2025 01:19
@trask trask force-pushed the copilot-code-review-test branch 2 times, most recently from 2c76010 to 5d3375f Compare August 31, 2025 19:52
@trask trask force-pushed the copilot-code-review-test branch from 5d3375f to e514e99 Compare September 1, 2025 00:12
@zeitlinger
Copy link
Member

looks like improving @Nullable (or adding NullAway) is still too hard - right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breedx-splk breedx-splk breedx-splk left review comments

@atoulme atoulme Awaiting requested review from atoulme

@HaloFour HaloFour Awaiting requested review from HaloFour

@jack-berg jack-berg Awaiting requested review from jack-berg

@jackshirazi jackshirazi Awaiting requested review from jackshirazi

@jkwatson jkwatson Awaiting requested review from jkwatson

@JonasKunz JonasKunz Awaiting requested review from JonasKunz

@jsuereth jsuereth Awaiting requested review from jsuereth

@LikeTheSalad LikeTheSalad Awaiting requested review from LikeTheSalad

@MikeGoldsmith MikeGoldsmith Awaiting requested review from MikeGoldsmith

@psx95 psx95 Awaiting requested review from psx95

@robsunday robsunday Awaiting requested review from robsunday

@sfriberg sfriberg Awaiting requested review from sfriberg

@srprash srprash Awaiting requested review from srprash

@SylvainJuge SylvainJuge Awaiting requested review from SylvainJuge

@wangzlei wangzlei Awaiting requested review from wangzlei

@zeitlinger zeitlinger Awaiting requested review from zeitlinger

+1 more reviewer

@KarstenSchnitter KarstenSchnitter KarstenSchnitter left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@atoulme atoulme

@jsuereth jsuereth

@jkwatson jkwatson

@KarstenSchnitter KarstenSchnitter

@JonasKunz JonasKunz

@HaloFour HaloFour

@jackshirazi jackshirazi

@jack-berg jack-berg

@LikeTheSalad LikeTheSalad

@breedx-splk breedx-splk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.