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

Instrumentation API part 2 to main #416

Merged
merged 2 commits into from
Jun 14, 2024
Merged

Instrumentation API part 2 to main #416

merged 2 commits into from
Jun 14, 2024

Conversation

LikeTheSalad
Copy link
Contributor

Supersedes #415

@LikeTheSalad
Instrumentation API part 2 (open-telemetry#408)
42635e1 
* Moving network provider tools to services

* Moving network service related tests

* Moving network attr init feature to the agent

* Making network instrumentation depend on the agent

* Initializing CurrentNetworkProvider as service

* Clean up config

* Created NetworkChangeInstrumentation

* Updated network monitor tests

* Clean up

* Renaming installOn by start

* Adding internal class docs

* Fixing visible for tests comment

* Rename .java to .kt

* Removing Carrier.Builder
@LikeTheSalad
Merge branch 'main' into main-to-instrumentation-api
48b9829 
# Conflicts:
#	instrumentation/network/src/test/java/io/opentelemetry/android/instrumentation/network/NetworkChangeMonitorTest.java
@LikeTheSalad LikeTheSalad requested a review from a team June 6, 2024 12:31
@LikeTheSalad LikeTheSalad changed the base branch from main to feature/instrumentation-api June 6, 2024 12:32
@LikeTheSalad LikeTheSalad mentioned this pull request Jun 6, 2024
Closed
@breedx-splk
Copy link
Contributor

There look to be a bunch of unrelated changes in this PR. @LikeTheSalad can you look into that plz?

@LikeTheSalad
Copy link
Contributor Author

There look to be a bunch of unrelated changes in this PR. @LikeTheSalad can you look into that plz?

Do you mean changes that are not in the main branch? If you could point to one in specific @breedx-splk I'll have a deeper look tomorrow

@breedx-splk
Copy link
Contributor

There look to be a bunch of unrelated changes in this PR. @LikeTheSalad can you look into that plz?

Do you mean changes that are not in the main branch? If you could point to one in specific @breedx-splk I'll have a deeper look tomorrow

Yeah, there are a TON. Like all the demo app stuff, and some of the semconv stuff from earlier. Just look at the files list and you'll see what I'm talking about.

@LikeTheSalad
Copy link
Contributor Author

Yeah, there are a TON. Like all the demo app stuff, and some of the semconv stuff from earlier. Just look at the files list and you'll see what I'm talking about.

I see. Yeah all the semconv changes and the demo app ones have been recently merged into main, so I think it makes sense that they show up here because I'm bringing all the latest changes from main into the feature/instrumentation-api branch. My goal with this PR is to keep the feature branch updated with the latest changes from main, otherwise I could run into a lot of git conflicts later given that the main branch is changing a lot these days. So for example, in this PR is where the semconv changes were added into main, a lot of the changes made there are affecting the instrumentation modules so that's one of the reasons why I'd like to bring those changes into the feature branch before starting to migrate those modules, otherwise when it comes the time to merge the feature branch into main, that PR is going to be full of git conflicts if main changed too much while the feature branch was getting ready, and that's what I'd like to avoid by bringing the latest changes from main into the feature branch as often as possible.

@breedx-splk
Copy link
Contributor

Oh, I think I completely missed the fact that the part 1 and part 2 PRs (and now part 3) were going against this branch. Why do you want to build up all those compounded changes into a branch before merging to main? I'm not seeing the benefit. I'd much rather review and merge smaller PRs into main.

Having the changes in this other branch also means that the same changes are getting reviewed twice, as well as all these redundant changes from main (semconv, demo app, etc). How do you propose that we review this?

@LikeTheSalad
Copy link
Contributor Author

Oh, I think I completely missed the fact that the part 1 and part 2 PRs (and now part 3) were going against this branch.

Ah, got it, my bad, I think I didn't explain it properly in the previous PR. Part 1 was merged into main, which I guess makes things even more confusing 😅

Ok so what happened was that, when I was working on part 2, I noticed that those changes would remove one of the current auto instrumentations provided in the agent (the network change one) because it was removed from the android-agent dependencies. Said instrumentation would be re-added as a default one by the end of the whole instrumentation API work, but in the meantime, it was not going to be available due to some incompatibilities with how the old vs the new instrumentation API works.

Based on that, I was concerned that the new instrumentation API work would take too long, causing that, should we create a new build in the meantime, it wouldn't have some of the instrumentations available and I didn't want to cause issues for someone using it now until it was all ready to mention the breaking change in a single build release. This is why I decided to keep all the parts in a single feature branch until it was all ready to finally add all the changes into main, and not some unfinished work.

Having the changes in this other branch also means that the same changes are getting reviewed twice, as well as all these redundant changes from main (semconv, demo app, etc). How do you propose that we review this?

This is a good point, I think I didn't think this part through, I was mostly focusing on not breaking main while I was working on all the parts, because the combination of all of those parts is needed to keep the same default instrumentations working out of the box. However, I agree it could just complicate things down the road, with keeping up the feature branch with the latest changes from main, as well as having to kind of review things twice in the end. Maybe I was just overthinking this tbh. With the current pace at which I've gotten reviews for the parts that I've created so far, probably the whole work wouldn't take too long and we could wait until all of them are merged before creating a new build.

@LikeTheSalad LikeTheSalad changed the base branch from feature/instrumentation-api to main June 11, 2024 07:55
@LikeTheSalad LikeTheSalad changed the title Main to instrumentation api Instrumentation API part 2 to main Jun 11, 2024
@LikeTheSalad
Copy link
Contributor Author

@breedx-splk Based on the above, I've just changed the base branch of this PR to main, to merge the "part 2" into main to continue there with further parts as well.

breedx-splk
breedx-splk reviewed Jun 13, 2024
View reviewed changes
...id-agent/src/main/java/io/opentelemetry/android/internal/services/network/CarrierFinder.java
Comment on lines +27 to +30
String name = null;
String mobileCountryCode = null;
String mobileNetworkCode = null;
String isoCountryCode = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should probably move these declarations closer to their usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are used in line 44 but they can change before reaching that line, that's why they're not close to line 44 because of the "ifs" needed to re-set them before that.

breedx-splk
breedx-splk approved these changes Jun 13, 2024
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.

Déjà vu!! 😁

@LikeTheSalad LikeTheSalad merged commit 99f3189 into open-telemetry:main Jun 14, 2024
3 checks passed
@LikeTheSalad LikeTheSalad deleted the main-to-instrumentation-api branch June 14, 2024 05:56
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 approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@LikeTheSalad @breedx-splk