-
Notifications
You must be signed in to change notification settings - Fork 42
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.
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
Instrumentation API - Part 1 #396
Instrumentation API - Part 1 #396
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Off to a great start here! Had a few comments, but nothing to slow down this effort. Thanks.
android-agent/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt
Outdated
Show resolved
Hide resolved
android-agent/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentation.kt
Outdated
Show resolved
Hide resolved
...ent/src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistry.kt
Outdated
Show resolved
Hide resolved
...src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImpl.kt
Show resolved
Hide resolved
...src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImpl.kt
Show resolved
Hide resolved
...test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImplTest.kt
Outdated
Show resolved
Hide resolved
...test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImplTest.kt
Outdated
Show resolved
Hide resolved
…tation/AndroidInstrumentation.kt Co-authored-by: jason plumb <[email protected]>
…tation/AndroidInstrumentation.kt Co-authored-by: jason plumb <[email protected]>
…tation/AndroidInstrumentationRegistry.kt Co-authored-by: jason plumb <[email protected]>
override fun getAll(): Collection<AndroidInstrumentation> { | ||
return Collections.unmodifiableCollection(instrumentations.values) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to use the Java Collection
types instead of the Kotlin ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean kotlin.collections.Collection
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, the kotlin.collections
package in general.
The less we depend on java.*
the easier it'll be to be KMP compatible in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I was looking for a Collections.unmodifiableCollection
equivalent, though it seems like toList
should do. I've just made the changes. Cheers!
...src/main/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImpl.kt
Show resolved
Hide resolved
...test/java/io/opentelemetry/android/instrumentation/AndroidInstrumentationRegistryImplTest.kt
Outdated
Show resolved
Hide resolved
…tation/AndroidInstrumentationRegistryImpl.kt Co-authored-by: Manoel Aranda Neto <[email protected]>
First part - Creating the API