Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1-beta2
    • Component/s: HttpCore
    • Labels:
      None

      Description

      I found this issue HTTPCORE-181 raised before.
      I got the similar requirement that we want to measure all kinds of time-related data to get the performance report such as:
      1. DNS parsing time
      2. Connection setup time
      2. header parsing time
      3. body transfer time
      ...
      I found that there are a lot of "private" fields which have no setter method in this component, this makes us hard to extend the functionality.
      For example in org.apache.http.impl.io.AbstractSessionInputBuffer#init() method, it would be better to change "this.metrics = new HttpTransportMetricsImpl();" to "this.metrics = createMetrics()" (a protected method) which subclasses can easily override this if they want to get an extended HttpTransportMetricsImpl instance.

      Can some one look at this feature?

        Activity

        Hide
        Oleg Kalnichevski added a comment -

        #createMetrics() protected method can be added no problem. What other changes would you envisage?

        Oleg

        Show
        Oleg Kalnichevski added a comment - #createMetrics() protected method can be added no problem. What other changes would you envisage? Oleg
        Hide
        Zhang Guilin added a comment -

        Generally speaking, I think it would be better if we have more protected fields or methods rather than private ones. So that we can easily extend some concrete classes to meet our application's requirement.
        To be more specific, here is an example:
        In org.apache.http.impl.io.HttpResponseParser class, #parseHead(), "int i = sessionBuffer.readLine(this.lineBuf);" indicates that header transfer has been started and first line is received, it would be better to add a hook method or raise an event(might be more complicated) to do something. For example, add a protected method "headerTransferStarted" and append it behind "int i = sessionBuffer.readLine(this.lineBuf);" would be good.
        So in our application, we might want to record the header transfer start time, we can just do something like this:
        protected void headerTransferStarted()

        { headerStartTime = System.nanoTime(); }
        Show
        Zhang Guilin added a comment - Generally speaking, I think it would be better if we have more protected fields or methods rather than private ones. So that we can easily extend some concrete classes to meet our application's requirement. To be more specific, here is an example: In org.apache.http.impl.io.HttpResponseParser class, #parseHead(), "int i = sessionBuffer.readLine(this.lineBuf);" indicates that header transfer has been started and first line is received, it would be better to add a hook method or raise an event(might be more complicated) to do something. For example, add a protected method "headerTransferStarted" and append it behind "int i = sessionBuffer.readLine(this.lineBuf);" would be good. So in our application, we might want to record the header transfer start time, we can just do something like this: protected void headerTransferStarted() { headerStartTime = System.nanoTime(); }
        Hide
        Oleg Kalnichevski added a comment -

        Protected visibility has compatibility implications. Once a method is made protected it can no longer be changed without breaking the binary compatibility. I deeply regret having so MANY protected methods and variables should have been private instead.

        When doing something special one should consider building a custom implementation of relevant interface instead of an existing class and thus creating a dependency on a particular implementation. Besides, one would be much better off adding even notifications to an interface by decorating it with a wrapper class instead of extending a particular implementation of that interface.

        Protected methods similar to #createMetrics can be created on a case by case basis where justified.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Protected visibility has compatibility implications. Once a method is made protected it can no longer be changed without breaking the binary compatibility. I deeply regret having so MANY protected methods and variables should have been private instead. When doing something special one should consider building a custom implementation of relevant interface instead of an existing class and thus creating a dependency on a particular implementation. Besides, one would be much better off adding even notifications to an interface by decorating it with a wrapper class instead of extending a particular implementation of that interface. Protected methods similar to #createMetrics can be created on a case by case basis where justified. Oleg
        Hide
        Zhang Guilin added a comment -

        I made a sample patch for time related metrics.
        Please feel free to let me know if I am trying to implement something in a bad way.

        Show
        Zhang Guilin added a comment - I made a sample patch for time related metrics. Please feel free to let me know if I am trying to implement something in a bad way.
        Hide
        Oleg Kalnichevski added a comment -

        I see nothing wrong with the patch but I do not think it should be committed. Performance metrics are not particularly cheap and often are useful only when troubleshooting performance / networking issues. They should not be a part of the framework, but should be added when really required through custom extensions. In your particular case you can avoid having to patch HttpCore code by creating a custom response parser which extends AbstractMessageParser.

        Oleg

        Show
        Oleg Kalnichevski added a comment - I see nothing wrong with the patch but I do not think it should be committed. Performance metrics are not particularly cheap and often are useful only when troubleshooting performance / networking issues. They should not be a part of the framework, but should be added when really required through custom extensions. In your particular case you can avoid having to patch HttpCore code by creating a custom response parser which extends AbstractMessageParser. Oleg
        Hide
        Zhang Guilin added a comment -

        OK,I got it.
        You are right, thanks. I decide to do it in our application.

        Show
        Zhang Guilin added a comment - OK,I got it. You are right, thanks. I decide to do it in our application.
        Hide
        Oleg Kalnichevski added a comment -

        Good. Do you still want #createMetrics method added?

        Oleg

        Show
        Oleg Kalnichevski added a comment - Good. Do you still want #createMetrics method added? Oleg
        Hide
        Zhang Guilin added a comment -

        Yes, it would be better if it can be added.
        Besides this, I think creating HttpConnectionMetrics also has similar situation.

        Show
        Zhang Guilin added a comment - Yes, it would be better if it can be added. Besides this, I think creating HttpConnectionMetrics also has similar situation.
        Hide
        Oleg Kalnichevski added a comment -

        Methods added to relevant classes in SVN trunk. Please review.

        Oleg

        Show
        Oleg Kalnichevski added a comment - Methods added to relevant classes in SVN trunk. Please review. Oleg

          People

          • Assignee:
            Unassigned
            Reporter:
            Zhang Guilin
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development