Cloudlog icon indicating copy to clipboard operation
Cloudlog copied to clipboard

Reduce calling scp, cache result

Open DJ3CE opened this issue 3 months ago • 15 comments

For callsign suggestions in logging, Cloudlog requested a list from the server with every additional character typed, from a minimum of 3 on.

This PR reduces calls to the server, by saving and filtering a (matching) list from the server for additional characters typed.

Example: You typed 'DJ3' and got a list of all callsigns. Now you add 'C' ('DJ3C') and while previously, the server was asked to give a new suggestion list with this PR the already received list is filtered for 'DJ3C'.

If the original request does not match the typed callsign anymore (Example: You replace the '3' in the previous example by a '4'), a new request is made.

Included in this PR is contest-, as well as normal-logging.

DJ3CE avatar Mar 24 '24 23:03 DJ3CE

I think just adding a debounce might be a better solution:

// Debounce function
function debounce(func, wait) {
    var timeout;
    return function() {
        var context = this, args = arguments;
        clearTimeout(timeout);
        timeout = setTimeout(function() {
            func.apply(context, args);
        }, wait);
    };
}

// On Key up check and suggest callsigns
$("#callsign").keyup(debounce(function () {
    var call = $(this).val();
    if (call.length >= 3) {

        $.ajax({
            url: 'lookup/scp',
            method: 'POST',
            data: {
                callsign: $(this).val().toUpperCase()
            },
            success: function (result) {
                $('.callsign-suggestions').text(result);
                highlight(call.toUpperCase());
            }
        });
        // moved to blur
        // checkIfWorkedBefore();
        var qTable = $('.qsotable').DataTable();
        qTable.search(call).draw();
    }
    else if (call.length <= 2) {
        $('.callsign-suggestions').text("");
    }
}, 500)); // 500ms delay

~not sure if this pseudo code works, but the idea is to just wait some delay for the user to stop typing before firing off a request~

Tested it locally and it works, just not sure what the best delay amount would be, this was a test w/ a 3 second delay just to ensure the code is working..itd probably be better at like ~1 second also we should add a spinner next to the input so the user knows theres a request being sent:

3-26-2024 (21-31-33)

patrickrb avatar Mar 27 '24 02:03 patrickrb

Not sure if we want to solve the same problem ;). I'd go for trying to get suggestions as early and fast as possible and narrow them down as fast as possible, therefore it seems counterintuitive to do 'debouncing', but more natural to filter stored results.

DJ3CE avatar Mar 27 '24 06:03 DJ3CE

So based on the pr description, I thought we were trying to reduce the number of requests sent from the frontend to the backend as the user types.

For callsign suggestions in logging, Cloudlog requested a list from the server with every additional character typed, from a minimum of 3 on. This PR reduces calls to the server,

The debounce solution is the standard go to way of preventing sending a request as a user types until they are done typing.

I think implementing caching here is setting up for problems down the road.

There's an old joke in programming:

There are only two hard problems in computer science: Cache invalidation, naming things, and off-by-one errors.

patrickrb avatar Mar 27 '24 11:03 patrickrb

... but the cited sentence has some more words to it ;).

I'm totally fine with alternative solutions taking other priorities in server-load and user-experience, but I guess here I'd appreciate an eye on this particular approach - especially, as pointed out, since cache invalidation is not the easiest thing to do. Feel free to point out gaps I might have left.

The approach taken just uses a state-model representing if and for which characters a request is on its way.

DJ3CE avatar Mar 27 '24 11:03 DJ3CE

... one thing to add: I guess there could be different solutions 'right' for 'normal' and for 'contest' logging.

Based on your screenshots you seem to focus on 'normal'-logging, while I started implementing this for contest-logging.

DJ3CE avatar Mar 27 '24 11:03 DJ3CE

I definitely see where you're coming from, and caching is a valid way of solving the stated problem.

Both solutions solve the problem, and can even work in tandem together.

With that said, in my personal experience, it would best to start with the debounce method at ~300-500ms delay (which isn't even noticeable to the end user, it's just enough to prevent multiple calls if the user mistypes and quickly fixes).

The main reasons to start with debounce over cache are:

  1. Ease of Implementation: Debouncing can be implemented quickly, This makes it a low-effort, high-impact change.
  2. Minimal Risk: Since debouncing is applied on the frontend and primarily affects how often the application makes requests to the backend, it introduces minimal risk to existing backend logic or data integrity.

If we implement debounce and *you're still not happy with how it solves the problem, then I think it'd be the appropriate time to look at integrating some type of caching.

I hope this clears up where I'm coming from and the best practice for order of operations on a problem/solution like this.

patrickrb avatar Mar 27 '24 12:03 patrickrb

For SCP you want to get results as fast as possible this obviously is different for the QSO box fields some debounce on those would be a great idea.

magicbug avatar Mar 28 '24 14:03 magicbug

This PR does seem to have an issue for example with my callsign when I type 2M0 no SCP results ever appear but GM1 etc function as expected

magicbug avatar Mar 28 '24 14:03 magicbug

This PR does seem to have an issue for example with my callsign when I type 2M0 no SCP results ever appear but GM1 etc function as expected

Thanks for trying this out and pointing me to issues where '0' is part of the callsign. What is the convention about the use of '0' or 'Ø' in callsigns in Cloudlog?

DJ3CE avatar Mar 29 '24 10:03 DJ3CE

There you go. Thank you for input and testing, appreciate it.

  • @magicbug callsigns including '0' now work :). The returned list hast stroked-0 in it, thus the filtering failed (miserably ;) ),
  • instead of String.includes() a regex is used, to have prefixed callsigns in the list while not being too coarse,
  • scp_data.status got discarded,
  • 'rough' network conditions are handled better by asking if a server-response still matches the currently entered part-of-a-callsign

DJ3CE avatar Mar 29 '24 15:03 DJ3CE

@magicbug, please let me know if there's anything which prevents merging.

DJ3CE avatar Apr 01 '24 12:04 DJ3CE

Please read up on javascript best practices before refusing changes that catch the code up to javascript best practices.

We should ALWAYS use === checks.

We should ALWAYS use let and const, NEVER var.

patrickrb avatar Apr 01 '24 13:04 patrickrb

Feel free not to use the .abort logic if you think its too complex, but the rest of the keyUp function should look like this:

// On Key up check and suggest callsigns
$("#callsign").keyup(() => {
	const call = $(this).val().toUpperCase();
	const cleanedCall = call.replace('0', 'Ø');

	// Exit early if the users starts typing a completely new call
	if (scp_data.previousRequest && !call.startsWith(scp_data.request)) {
		scp_data.previousRequest.abort();
	}

	if (call.length >= 3) {
		if (scp_data.request === "" || !call.startsWith(scp_data.request)) {
			scp_data.request = call;
			scp_data.data = [];
			scp_data.previousRequest = $.ajax({
				url: 'lookup/scp',
				method: 'POST',
				data: {
					callsign: call
				},
				success: function (result) {
					const currentCall = $("#callsign").val().replace('0', 'Ø').toUpperCase();
					if (currentCall.startsWith(call)) {
						scp_data.data = result.split(" ");
						$('.callsign-suggestions').text(filterCallsignList(currentCall, scp_data.data));
						highlight(currentCall);
					}
				},
				complete: function () {
					scp_data.previousRequest = null;
				}
			});

		} else {
			// Filter the already obtained list:
			$('.callsign-suggestions').text(filterCallsignList(cleanedCall, scp_data.data));
			highlight(cleanedCall);
		}
		const qTable = $('.qsotable').DataTable();
		qTable.search(call).draw();
	}
	else {
		$('.callsign-suggestions').text("");
	}
});

It's inarguably cleaner than what've you've provided in your PR.

patrickrb avatar Apr 01 '24 14:04 patrickrb

This PR does seem to have an issue for example with my callsign when I type 2M0 no SCP results ever appear but GM1 etc function as expected

Thanks for trying this out and pointing me to issues where '0' is part of the callsign. What is the convention about the use of '0' or 'Ø' in callsigns in Cloudlog?

I'll be honest the convention in Cloudlog should be Ø but.. it varies perhaps something for improvement.

Can confirm that it now works, but we have lost the ability to just type the suffix and show matches based on that which is super handy with the SCP box

magicbug avatar Apr 02 '24 12:04 magicbug

I'll be honest the convention in Cloudlog should be Ø ...

Sounds like s.th. to work with and to have incremental improvement :).

Now replacing 'Ø' by '0' in controllers/Lookup.php:scp(), as both scp-files (clublog_scp.txt and masterscp.txt) and the db-table use '0', then it makes no difference how the backend gets called. The conversion back to 'Ø' is already done in scp(), an exception of '0's shows up with getSessionQsos in contesting.

Can confirm that it now works, but we have lost the ability to just type the suffix and show matches based on that which is super handy with the SCP box

Thanks for pointing that out, haven't observed nor used that up to now (I seem not to remember suffixes without prefixes :D), so now consistently switching to String.includes() for matching cache-validity and showing results. @patrickrb already mentioned includes(), but neither with the background @magicbug gave here nor consistent - sorry, haven't seen that before.

Is there a library-like js-file to have one place for code(-pieces) like this? I have separated the 'functional' code from its specific application to a keyup, so both, qso.js and contesting.js have the same functional code, which could be put in a single place elsewhere (... and no more global-variable is needed :) ).

Edit: I might add, that the check for cache validity got inverted, you'll directly get to the updated expression with de Morgan.

DJ3CE avatar Apr 04 '24 17:04 DJ3CE