session icon indicating copy to clipboard operation
session copied to clipboard

src: more debug statements

Open gireeshpunathil opened this issue 6 years ago • 7 comments

Add debug statement (enabled only in debug mode) at vital session life cycle control flow points

Refs: https://github.com/expressjs/session/issues/633#issuecomment-471110093

Generally improves ability to diagnose issues.

(from a high level view on the outstanding issues in this repo, it is evident that the most prominent type is diagnosing session persistence across client requests, so this is a small step towards helping there)

gireeshpunathil avatar Nov 04 '19 09:11 gireeshpunathil

  • I have addressed the review comments
  • I have added the word session in the debug statements in touch code
  • request to have a look
  • request to remove the low priority tag

gireeshpunathil avatar Jul 10 '20 14:07 gireeshpunathil

request to remove the low priority tag

For what reason? Do you think this is not a low priority against the other aspects the team is attempting to move forward like Express 5.0? I'm trying to apply tags around priorities to help people focus the appropriate attention to get larger efforts like 5.0 moving forward. IMO adding some extra debug wording seems low priority to me, compared to things like new features, bug fixes, etc.

The tag itself doesn't mean that it gets no attention, just that it is comparatively low priority. I'm happy to remove it, but just unsure on the reasoning for removing it, as you didn't provide any.

dougwilson avatar Jul 10 '20 14:07 dougwilson

I am not arguing with you; but I cannot find a reason why this is a low priority: the reasoning for this change is provided in the first comment - to improve self diagnosis of a class of issues that report malfunction of session life-cycle events, more specifically users suspecting missing session persistence. Plus, there are 27 other PRs in this repo, and none of those are low pririty items!

But I am defenitely not arguing with you nor questing your judgement. Also, if this is to compete with express 5.0 for project's attention and focus, I will prefer the later, as that has more user demand than the former.

gireeshpunathil avatar Jul 10 '20 15:07 gireeshpunathil

I already provided the reason why it is low priority above :) The tag has nothing to do with the timeline for landing the change; it is simply to provide a category of the change itself. The argument that the others are not tagged right is not an argument this is not a low priority change; getting things tagged correctly is actually your primary function as a triager :) So if other PRs are not tagged correctly, it sounds like that is an issue that needs t be addressed with at least the triagers; I was simply tagging this PR since I happened to just interact with it. If it is more important for me to go through all the issues and PRs in this repository and get them all tagged appropriately vs working on other items, I can certainly do that, but the idea was that we're bringing on the large group of triagers to perform these kinds of tasks such that folks like myself can actually focus on the larger-at-hand items.

dougwilson avatar Jul 10 '20 15:07 dougwilson

I already provided the reason why it is low priority above :)

I have given response to that - it is not some random debug statements; it is derived from an insight from this repo's issue backlog.

The argument that the others are not tagged right is not an argument this is not a low priority change; getting things tagged correctly is actually your primary function as a triager :)

There are PRs which are 6 years old with no activities for as long; and acting on those in a meaningful and timely manner is actually your primary function as a maintainer :)

But then let us agree that we are not here to remind each other on their responsibilities.

So if other PRs are not tagged correctly, it sounds like that is an issue that needs t be addressed with at least the triagers; I was simply tagging this PR since I happened to just interact with it.

It looks incorrect approach to me. If you just happened to interact with a work item cannot be used to derive a priority decision? Priorities are always relative; without looking at other PRs, a 'low priority' item cannot stand in itself. Low against what?

gireeshpunathil avatar Jul 10 '20 15:07 gireeshpunathil

I have given response to that - it is not some random debug statements; it is derived from an insight from this repo's issue backlog.

I get it, but we're also not even actively telling folks to use what is there when triaging issues (see recent issues like #763 )

I think that this conversation is getting a bit out of hand and off topic, and frankly into a bit of the offensive realm. If you could, I would suggest if you could please step away for a bit. I'm going to lock this PR for now.

dougwilson avatar Jul 10 '20 15:07 dougwilson

@gireeshpunathil I think at this point going forward, I just can no longer act on any issues and PR you raise. You will need to get something else involved to move forward with your issues and PRs, as I do not feel comfortable in interactions between us.

dougwilson avatar Jul 10 '20 15:07 dougwilson