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

RUMM-407 Use app package name as default service name value #241

Merged
merged 1 commit into from
Apr 30, 2020
Merged

RUMM-407 Use app package name as default service name value #241

merged 1 commit into from
Apr 30, 2020

Conversation

mariusc83
Copy link
Member

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@mariusc83 mariusc83 requested a review from a team as a code owner April 28, 2020 13:58
@mariusc83 mariusc83 self-assigned this Apr 28, 2020
xgouchet
xgouchet requested changes Apr 28, 2020
View reviewed changes
dd-sdk-android/src/main/kotlin/com/datadog/android/DatadogConfig.kt Outdated
val serviceName: String,
val envName: String
val envName: String,
val serviceName: String? = null
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid changing parameters order, this is a nightmare whenever we have a rebase/merger afterwards.
Also you don't need a default value here, the default will come from the config builder

dd-sdk-android/src/main/kotlin/com/datadog/android/DatadogConfig.kt
DEFAULT_ENV_NAME
)
private var tracesConfig: FeatureConfig = FeatureConfig(
clientToken,
applicationId,
DatadogEndpoint.TRACES_US,
DEFAULT_SERVICE_NAME,
DEFAULT_ENV_NAME
)
Copy link
Member

Choose a reason for hiding this comment

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

you can set the default value here, that way you don't have to switch the order.

dd-sdk-android/src/main/kotlin/com/datadog/android/tracing/internal/TracesFeature.kt Outdated
@@ -58,7 +58,7 @@ internal object TracesFeature {

clientToken = config.clientToken
endpointUrl = config.endpointUrl
serviceName = config.serviceName
serviceName = config.serviceName ?: appContext.packageName
Copy link
Member

Choose a reason for hiding this comment

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

Instead of copy/pasting this logic everywhere, you could define it once and for all in a CoreFeature.serviceName property, that'll be then used by other features ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was actually thinking the same...but CoreFeature doesn't hold a reference to the FeatureConfig or RumConfig. I would have to actually provide the FeatureConfig as a parameter to that method and because RumFeature has a different config (RumConfig) I will need 2 methods. I am not really sure which approach will be better: having the code duplicated in each feature or having a 2 methods in the CoreFeature :(. Maybe I can think to something else.

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-407/change-default-value-for-service-name branch 2 times, most recently from 091529e to d6dbf29 Compare April 29, 2020 11:39
@github-actions github-actions bot added the api-surface-change This PR introduce a change in the API surface label Apr 29, 2020
xgouchet
xgouchet requested changes Apr 29, 2020
View reviewed changes
dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt Outdated
@@ -56,25 +57,27 @@ internal object CoreFeature {

internal var packageName: String = ""
internal var packageVersion: String = ""
private var customServiceName: String? = null
Copy link
Member

Choose a reason for hiding this comment

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

You didn't save any logic copy. You could do :

internal var serviceName: String = ""

fun initialize(…) {
    // …
    serviceName = config.serviceName ?: packageName
}

fun stop() {
    // …
    serviceName = ""
}

Then in all the other XFeature, remove the XFeature.serviceName properties and reference this one here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh...yes that would be better, don't know why I didn't think about this. Tks.

dd-sdk-android/src/test/kotlin/com/datadog/android/DatadogConfigBuilderTest.kt Outdated
fakeEnvName
)
)
}

@Test
fun `builder with custom service name`(
Copy link
Member

Choose a reason for hiding this comment

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

Why was this test was 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.

Oups.....I lost this during a rebase, I thought I put it back. Good spot.

dd-sdk-android/src/test/kotlin/com/datadog/android/error/internal/CrashReportsFeatureTest.kt Outdated
@@ -250,4 +249,64 @@ internal class CrashReportsFeatureTest {
assertThat(endpointUrl).isSameAs(endpointUrl2)
assertThat(serviceName).isSameAs(serviceName2)
}

@Test
fun `will use the custom service name if provided`(forge: Forge) {
Copy link
Member

Choose a reason for hiding this comment

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

If you follow my suggestion above, you only need to test the service name resolution in the CoreFeatureTest file

Copy link
Member Author

Choose a reason for hiding this comment

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

But still I want to keep these tests as this will guarantee that the serviceName will be set in these features and their default value will be the one from the CoreFeature

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-407/change-default-value-for-service-name branch from d6dbf29 to e531236 Compare April 30, 2020 07:44
xgouchet
xgouchet reviewed Apr 30, 2020
View reviewed changes
dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt Outdated
Comment on lines 113 to 116
fun resolveServiceName(): String {
return serviceName ?: packageName
}

Copy link
Member

Choose a reason for hiding this comment

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

You can delete this one, it's not used anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup..again forgot, damn it.

@mariusc83 mariusc83 force-pushed the mconstantin/rumm-407/change-default-value-for-service-name branch 2 times, most recently from 2f209f7 to e3eb2dd Compare April 30, 2020 08:22
@mariusc83 mariusc83 force-pushed the mconstantin/rumm-407/change-default-value-for-service-name branch from e3eb2dd to 3aa9f31 Compare April 30, 2020 08:34
@mariusc83 mariusc83 merged commit e12c633 into dd-rum Apr 30, 2020
@mariusc83 mariusc83 deleted the mconstantin/rumm-407/change-default-value-for-service-name branch April 30, 2020 11:08
@ncreated ncreated mentioned this pull request May 11, 2020
Merged
3 tasks
@xgouchet xgouchet added this to the 1.5.0 milestone May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xgouchet xgouchet xgouchet approved these changes

Assignees

@mariusc83 mariusc83

Labels
api-surface-change This PR introduce a change in the API surface
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
2 participants
@mariusc83 @xgouchet