search

open-telemetry / opentelemetry-android

OpenTelemetry Tooling for Android
Apache License 2.0
147 stars 33 forks source link

Instrumentation API part 2 to main #416

Closed LikeTheSalad closed 3 months ago

LikeTheSalad commented 4 months ago

Supersedes https://github.com/open-telemetry/opentelemetry-android/pull/415

breedx-splk commented 4 months ago

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

LikeTheSalad commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

@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.