Feature management for #88 by olivierperez · Pull Request #141 · ChuckerTeam/chucker

olivierperez · 2019-10-15T12:22:59Z

💎 Example

Kotlin

configureChucker {
    http {
        enabled = true
        showNotification = true
        retentionPeriod = RetentionManager.Period.ONE_HOUR
        maxContentLength = 250000L
        headers {
            redact("Authorization")
            redact("Auth-Token")
            redact("User-Session")
        }
    }
    error {
        enabled = true
    }
}

Java

HashSet<String> headersToRedact = new HashSet<>();
headersToRedact.add("Authorization");
headersToRedact.add("Auth-Token");
headersToRedact.add("User-Session");

List<TabFeature> features = Arrays.asList(
        new HttpFeature(
                true,
                true,
                RetentionManager.Period.ONE_HOUR,
                DEFAULT_MAX_CONTENT_LENGTH,
                headersToRedact
        ),
        new ErrorsFeature(
                true,
                true
        )
);

ChuckerJavaConfig.configure(features);

📷 Screeshots

📄 Context

Some integrators want to be able to disable the errors feature Provide an option to remove "errors" tab #88

📝 Changes

FeatureManager is an object that handle the Features system
- It helps to know which features are enabled, and get their configurations
Feature interface describe what is a Feature
- Implemented by HttpFeature and ErrorsFeature
Every code that concerns HTTP or ERRORS are now calling FeatureManager to check if feature is enabled

📎 Related PR

Add an option to remove "errors" tab #100 Was offering another way to achieve it, but last was done on Aug, 20

⚠️ Breaking

None

📝 How to test

Change configuration in sample -> ChuckerApplication

🔮 Next steps

I still have to add the DSL in library-no-op 🤷‍♂️

vbuberen · 2019-10-17T22:55:53Z

-                transaction.isResponseBodyPlainText = false
-            }
-        }
+        return if (httpFeature.enabled) captureTransaction(request, chain)


What do you think about keeping the same style for if else everywhere?
For example, in the same file there are lines the conditions have braces for if and else cases (like on line 127).
I believe that it could improve readability a little bit.

We can enable the MandatoryBracesIfStatements detekt rule if you think it's the case

olivierperez · 2019-10-24T17:49:51Z

⚠️ I haven't put the maxContentLength and headersToRedact, maybe I should. What to you think of it @cortinico @redwarp?

cortinico

Commenting here to give an high-level feedback on the PR.
First, thanks for taking the time to write it <3
I do like the DSL and I think it clarifies the configuration block a lot :)

On the other hand, I think we should try to:

Don't introduce a strong API change. We're changing the public API of Chucker (changes in the constructors of ChuckerInterceptor and similar classes) in a significant way. The problem is that, while the DSL is really elegant to call in Kotlin, that's not the same for Java users. Moreover, we just released 3.x and it's probably too early to do a strong API change. What do you folks think? I'd rather try to create a DSL that can be optionally used instead of the canonical API used to initialize Chucker.
Rename Feature. I'd rather call it Tab or TabFeature as the public API it exposes is clearly related to a Tab (e.g. newFragment). A Feature interface is probably still useful but it should probably be more abstract (e.g. only name and and enabled would suffice). This would allow for further extensions with features that are not necessarily tabs.
Think about those notImplemented() in the -no-op library. Historically we used to use zero values (e.g. override val name: Int = 0). Is there a reason why now we're shifting our approach for the -no-op library?

cortinico · 2019-10-24T22:36:37Z

Tag is not really clear here what exactly it is

I've renamed it to id, is it clearer? Each feature should have a unique id.

- maxContentLength - headersToRedact

olivierperez · 2019-11-02T04:36:55Z

@cortinico
I've read your comments and proposed a fix almost each of them.

I still have to:

find a nice way avoid big breaking changes
offer an API for Java developers to configure Chucker

cortinico · 2019-11-04T00:43:47Z

Awesome, please let me know if you need some support :)

olivierperez · 2019-11-04T20:43:29Z

@cortinico Done !
I've:

Add ChuckerJavaConfig (see Example above)
Reworked the DSL to redact headers (tell me what version do you prefer)

vbuberen · 2020-01-03T13:24:29Z

     * @param transaction The HTTP transaction sent
     */
    internal fun onRequestSent(transaction: HttpTransaction) {
+        if (!httpFeature.enabled) return


I suggest to add braces here for readability and to match other code lines, like below where condition with only one line in it has braces.

I use braces when there is something to do.
My point of view is this is an early return, in order to do nothing. If I do nothing I have no braces 🤷‍♂

What do you think of that?

I use braces when there is something to do.

Agree with @olivierperez
Also the style guide here https://developer.android.com/kotlin/style-guide#braces allows single-line if statements like:

if (condition) effect

vbuberen · 2020-01-03T13:24:49Z

     * @param transaction The sent HTTP transaction completed with the response
     */
    internal fun onResponseReceived(transaction: HttpTransaction) {
+        if (!httpFeature.enabled) return


Same suggestion about braces as above.

vbuberen · 2020-01-03T13:25:51Z

 dependencies {
    implementation "com.squareup.okhttp3:okhttp:$okhttp3Version"
    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlinVersion"
+    implementation "androidx.fragment:fragment:$fragmentVersion"


Maybe we could think about some other way of implementing feature management, which would allow to not include this dependency?

cortinico · 2020-01-04T16:55:50Z

Thanks again for the big work @olivierperez

I have to be honest, I'm a bit hesitant on merging this. I have the feeling the whole approach is a bit over engineered. My point of view is that is a bunch of extra code that we will have to maintain for a limited benefit.

Ideally we should be able to have some sort of key-value storage used to store/retrieve configurations for specific features.

Your approach is based on having a FeatureManager that holds a set of TabFeature. Here we're already making the assumption that every feature is rendered using a Fragment in a ViewPager. This has several side effects:

The public API got way bigger (6 files were added in the -no-op library artifact).
The androidx.fragment:fragment dependency leaked inside the -no-op library. We should probably avoid having Fragments as part of the public API.
Having features that doesn't have a corresponding tab will require a refactoring, as well as features that are shared between two tabs.

Moreover, the DSL could be moved to a library-dsl artifact, and have it as a pluggable add-on.

I generally have the feeling that we should simplify a bit the whole approach.

On the other hand I strongly believe that we need some sort of modular infrastructure to make easy to Chucker extend.

cortinico · 2020-01-04T16:56:22Z

Also I invite the community, as well as @vbuberen @redwarp to provide their point of view on the topic :) Happy to discuss

vbuberen · 2020-01-05T11:02:10Z

From my point of view, this is too much for Chucker at the moment.
Having 2 tabs with transactions and errors isn't something too complex or requiring some additional efforts from users, who want just watch for transactions or for those, so want to only see throwable
errors.
Moreover, talking about errors - now errors are tracked by default, while with these changes user should explicitly write
error { enabled = true }. For me it seems more like adding more lines to set up the library than adding flexibility.
I agree with @cortinico here on being hesitant on merging this thing, since I am for as effortless experience as we can provide.

At the same time I should admit that a great amount of work was done here and thanks a lot for it, but we should probably do something like this later when we have more components, features, etc.

olivierperez · 2020-04-07T08:25:32Z

Hi, thanks for the review and the comments and sorry for the delay.
I think the way I choose is not really the right one. If we want to implement a way to configure features we should talk about it before I go develop it :-)

I propose to close this PR.

BTW It was fun to develop it anyway

cortinico · 2020-04-07T22:44:43Z

I propose to close this PR.
BTW It was fun to develop it anyway

Let's close it 👍 I anyway believe that we might come back to this topic at a certain topic and your PR could be used as material to kickoff the discussion again :)

olivierperez added enhancement New feature or improvement to the library Draft labels Oct 15, 2019

olivierperez changed the title ~~Feature management for !88~~ Feature management for #88 Oct 15, 2019

olivierperez force-pushed the feature_management branch 5 times, most recently from 3a3a574 to b440c8e Compare October 15, 2019 20:22

olivierperez removed the Draft label Oct 15, 2019

olivierperez requested review from cortinico and redwarp October 15, 2019 21:16

uOOOO mentioned this pull request Oct 17, 2019

Add an option to remove "errors" tab #100

Closed

vbuberen reviewed Oct 17, 2019

View reviewed changes

olivierperez force-pushed the feature_management branch from b440c8e to 1f31b8a Compare October 23, 2019 12:45

cortinico requested changes Oct 24, 2019

View reviewed changes

olivierperez force-pushed the feature_management branch from 2444d3d to 1f376bb Compare October 26, 2019 12:36

olivierperez added 9 commits November 2, 2019 04:53

Add a DSL to configure features HTTP / Errors

bc946fb

Show tabs for enabled features only

372a214

Some help for Travis-CI

d8bc980

Add DSL in no-op API

65f75b9

More help for Travis-CI

c49ec3a

End of Kotlinizing README

25f86cb

Add more stuff to DSL

95a5de9

- maxContentLength - headersToRedact

Update "Configuration" chapter with the DSL

ce97034

Fix tag of HttpFeature

ec9c9fe

olivierperez force-pushed the feature_management branch 2 times, most recently from 959e164 to 5997246 Compare November 2, 2019 04:00

Fix after Code review

b8ba648

olivierperez force-pushed the feature_management branch from 5997246 to b8ba648 Compare November 2, 2019 04:36

olivierperez force-pushed the feature_management branch from e8b4b8b to 88f4a66 Compare November 3, 2019 18:43

Avoid a big breaking change on ChuckerCollector and ChuckerInterceptor

0a5cefe

olivierperez force-pushed the feature_management branch from 88f4a66 to 0a5cefe Compare November 4, 2019 19:50

Add a way to configure Chucker for Java developers

cd4155e

olivierperez force-pushed the feature_management branch from ea5e371 to cd4155e Compare November 4, 2019 20:39

olivierperez force-pushed the feature_management branch 2 times, most recently from cce4ffd to fa092d0 Compare November 4, 2019 21:05

olivierperez added 2 commits November 4, 2019 22:18

Change way to redact headers in Kotlin DSL

cf1300d

Update README in regard of last changes

4fe7207

olivierperez force-pushed the feature_management branch from fa092d0 to 4fe7207 Compare November 4, 2019 21:18

olivierperez requested a review from cortinico November 4, 2019 21:27

vbuberen mentioned this pull request Nov 21, 2019

Feature/chucker configs #161

Closed

vbuberen reviewed Jan 3, 2020

View reviewed changes

cortinico closed this Apr 7, 2020

vbuberen deleted the feature_management branch April 20, 2020 09:40

MiSikora mentioned this pull request Sep 30, 2020

Create ChuckerInterceptor from builder #462

Closed

2 tasks

Uh oh!

Conversation

olivierperez commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💎 Example

Kotlin

Java

📷 Screeshots

📄 Context

📝 Changes

📎 Related PR

⚠️ Breaking

📝 How to test

🔮 Next steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olivierperez commented Oct 24, 2019

Uh oh!

cortinico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olivierperez commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortinico commented Nov 4, 2019

Uh oh!

olivierperez commented Nov 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cortinico commented Jan 4, 2020

Uh oh!

cortinico commented Jan 4, 2020

Uh oh!

vbuberen commented Jan 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olivierperez commented Apr 7, 2020

Uh oh!

cortinico commented Apr 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olivierperez commented Oct 15, 2019 •

edited

Loading

olivierperez commented Nov 2, 2019 •

edited

Loading

vbuberen commented Jan 5, 2020 •

edited

Loading