Internal Bugfix - add telemetry guards for string overflow #4538
Conversation
…toolkit-jetbrains into bugfix/telemetry-updates
| @@ -1,10 +0,0 @@ | |||
| <component name="CopyrightManager"> | |||
There was a problem hiding this comment.
NIT: Re-add this
.idea/copyright/apache.xml
Outdated
| @@ -1,6 +0,0 @@ | |||
| <component name="CopyrightManager"> | |||
There was a problem hiding this comment.
NIT: Re-add this
| object CodeTransformTelemetryMetadataSingleton { | ||
| private val instance = CodeTransformTelemetryMetadata() | ||
|
|
||
| fun getInstance() = instance |
There was a problem hiding this comment.
getInstance() in this project implies the platform "service" construct, rather than a singleton
There was a problem hiding this comment.
Are you saying the coding style for this repo of getInstance is reserved as a keyword for a "service" level construct? Or project level?
If its service level, then it would fit because this is a single instance shared across our service preventing duplicates.
There was a problem hiding this comment.
service as in the intellij platform service construct
|
|
||
| package software.aws.toolkits.jetbrains.services.codemodernizer.model | ||
|
|
||
| object CodeTransformTelemetryMetadataSingleton { |
There was a problem hiding this comment.
i like the modularization for testing, but now your project service is sharing state across all instances, which is probably not what you want
There was a problem hiding this comment.
We only have one instance at a time right now and we do want to share this across any reference to it. For simplicity we are not managing the state of the object within any of our manager classes.
If we move to a multi instance service, we can make this a variable with session or sessionContext.
There was a problem hiding this comment.
in the interest of avoiding global state that isn't managed by the platform then, can you convert this into a light service?
https://plugins.jetbrains.com/docs/intellij/plugin-services.html#light-services
your test will need to have an ApplicationRule applied
There was a problem hiding this comment.
Sure. I'll check out adding these changes to convert to:
@Service(Service.Level.PROJECT)
public final class MyProjectService {
...
| import junit.framework.TestCase.assertEquals | ||
| import org.junit.Test | ||
|
|
||
| open class CodeTransformTelemetryMetadataSingletonTest { |
There was a problem hiding this comment.
don't need to fix, but in future avoid unnecessary diffs
| import com.intellij.openapi.components.Service | ||
|
|
||
| @Service(Service.Level.APP) | ||
| public final class CodeTransformTelemetryService { |
There was a problem hiding this comment.
There was a problem hiding this comment.
instance for sure
getInstance is just a convenience to save needing to import two things every time. you are the only consumer of the service so it can go either way
|
Types of changes
Description
These changes are to prevent emitting telemetry items that are too long and is a defensive coding strategy to prevent overflows in the backend.
Checklist
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.