#metabrainz

/

      • CatQuest
        agrabaff, noone here speaks assamese?
      • lol trying to figure out if https://as.wikipedia.org/wiki/%E0%A6%8F%E0%A6%9... is about the lute, the string-drum or (like the english entry) both
      • ROpdebee has quit
      • thomasross has quit
      • _lucifer
        yvanzo: Freso: or any other JIRA admin, spam at BB-623
      • BrainzBot
        BB-623: Private Investigator in Delhi https://tickets.metabrainz.org/browse/BB-623
      • prabal joined the channel
      • prabal
        Who created this bookbrainz ticket? Private investigator?😂
      • CatQuest: my college was in assam. I don't speak Assamese, but i can ask someone to translate it
      • _lucifer
        alastairp: turns out using `url_for` in a background task is a hot mess. to use url_for, a request context must be available in addition to application context. in our case, the former isn't available. there is one fix, set SERVER_NAME, since we already have a SERVER_ROOT_URL config, it would be trivial to modify it to set SERVER_NAME. but setting SERVER_NAME has a side effect on how cookies work on subdomains.
      • cookies on beta.lb, test.lb and lb prod might interfere with each other if we do not configure it properly. for the time being, i think its best we revert to manually generating the url in spotify reader.
      • (we only use cookie for login, so login/logout on beta can also lead to the same happening on prod)
      • adhi001 joined the channel
      • ruaok, alastairp: ^, spotify reader got completely borked overnight.
      • shivam-kapila
        Private invigilator in Delhi. Wow next level spam
      • Good morning.
      • sumedh joined the channel
      • SothoTalKer has quit
      • SothoTalKer joined the channel
      • yvanzo
        _lucifer: thanks, deleted
      • adhi001 has quit
      • _lucifer
        yvanzo: thanks! can we also add the nicks of admins to the introduction section at jira homepage or maybe create a dedicated community post where users can report spam?
      • like aerozol had mentioned about the spam above but they did not know who or where to report it.
      • Mr_Monkey
        moin!
      • ruaok
        moin!
      • _lucifer: I got a suspcion that things were up last night. any idea on how to fix it?
      • _lucifer
        ruaok, have been trying to debug for some time no leads yet. it seems that the reader is unable to refresh expired tokens. why so no idea. i have reverted all containers prior to youtube PR release for now.
      • ruaok
        and the spice is flowing then?
      • _lucifer
        things may still be broken for users who modified sptoify settings between the initial release and now
      • ruaok
        eerrr, listens?
      • _lucifer
        yes
      • listens are being imported for ~1800 users currently
      • ruaok
      • a small downturn in listen recording.
      • yvanzo
        _lucifer: just added a line to the introduction text: spam can be reported under OTHER project.
      • _lucifer
        thanks yvanzo!
      • ruaok, yeah the spotify reader was broken ~8 hr last night and early morning today. and it still might be for a small number of users. so probably that's wehy.
      • also, since the timescale fixes were releases with youtube PR. they are also not in prod currently.
      • ruaok
        not a big deal.
      • let me wake up a bit more and I'll jump in to help think
      • _lucifer
        sure, thanks!
      • texke joined the channel
      • alastairp
        hi _lucifer
      • _lucifer
        Hi!
      • alastairp
        how can I help?
      • _lucifer
      • these are the relevant errors in the sentry.
      • today when i check spotify reader logs it was only running for 25 users.
      • I reverted spotify reader and prod to pre-youtube PR versions and it started to work fine, importing listens again for ~1800 users.
      • alastairp
        right, so looks like there are 2 things going on here? 1 is that it can't refresh tokens, and the other that when it tries to report, it can't generate the error messages?
      • _lucifer
        right, the unable to refresh tokens is the main issue.
      • alastairp
        so the things that have changed since the last release and this one is the switch to the new table, and the upgrade of spotipy?
      • _lucifer
        currently we are using old tables
      • alastairp
        sure - so the new tables don't work?
      • _lucifer
        right, new tables are not working as expected
      • they work as long as the access token works but once it expires things begin to fall apart
      • ruaok reads
      • alastairp
        so, the places where things may have gone wrong: copying to the new table; reading tokens from the new tables with the rewrites to the spotify importer; something unexpected in spotipy 2.17
      • _lucifer
        yes.
      • alastairp
        let me recreate the old environment locally and set up spotify
      • then I'll try the upgrade
      • _lucifer
        i have done that. unable to reproduce locally
      • alastairp
        interesting, so only some tokens are failing to refresh?
      • _lucifer
        yeah. the errors did not occur at once.
      • alastairp
        could it be something to do with scopes?
      • ruaok
        could we isolate the spotifpy update from our own tables update?
      • alastairp
        ruaok: yeah, that's a good idea
      • _lucifer: remind me - when we use a refresh token to get a new access token does the refresh token change? or just the access token?
      • ruaok
        because I really doubt it is our own tables. we'd catch an exception or whatnot.
      • _lucifer
        we do not use spotipy to refresh tokens, so i do not think that is the issue. but i am clueless currently so we can try
      • ruaok
        spotipy is a lot more opaque.
      • lets eliminate it for our own sanity.
      • _lucifer
        the refresh token may or may not change. we check the response and update out tables if spotify sends a new one
      • alastairp
        ok, cool
      • ruaok
        and you checked that the data written to DB is what you'd expect?
      • _lucifer
        this is the code to refresh the tokens https://github.com/metabrainz/listenbrainz-serv...
      • yeah, it worked for me locally. i could not check in prod because sentry filtered out access token in stack traces.
      • this is the current list of bugfixes i have done https://github.com/metabrainz/listenbrainz-serv...
      • ruaok
        thats a shitty combo of problems. :(
      • _lucifer
        the issue is we can mark an access token as expired in our db but spotify will accept it for another hour at least
      • so the feedback loop is slow...
      • should i deploy the bug fixes once and see if one of them fixes the issue?
      • ruaok
        is spotipy involved in determining if the access token expires?
      • > should i deploy the bug fixes once and see if one of them fixes the issue?
      • _lucifer
        yes, it spotipy throws a 401, that's how we know we need to refresh the token
      • ruaok
        yes, I think we should do that after we approve them.
      • could that be a bug in spotipy?
      • would it be possible to downgrade spotipy and test while alastairp and I review the proposed fixes?
      • _lucifer
        sure
      • ruaok
        thoughs alastairp ?
      • this line is interesting.
      • if there is no refresh token in the response, just continue using the old one.
      • _lucifer
        yeah, right. removed that in the proposed fixes.
      • ruaok
        ok, good.
      • alastairp
        was that in the original spotify reader?
      • ruaok
        yeah, if spotify throws some error at us we reuse the old token, but refresh its expiry time.
      • also, consider adding a logging hack for the time being.
      • a new function that takes a debug string, opens a file /tmp/lucifer.log and appends data to it.
      • _lucifer
        alastairp, it was like this earlier https://github.com/metabrainz/listenbrainz-serv...
      • ruaok
        then you can enter the debug container and see SOMETHING. hacky, but at least you get data.
      • -debug
      • alastairp
        _lucifer: ok, cool
      • I've got the old setup going, I'm having a poke around with the responses now
      • _lucifer
        alastairp: ruaok: i think i figured the issue. https://github.com/metabrainz/listenbrainz-serv...
      • we pass the access token to spotipy here
      • but after refreshing we never modify it again
      • ruaok
        and keep using the old token?
      • that would do it.
      • _lucifer
        so the token gets modified but the old token
      • yes that
      • this is some mess. it seems this was always the case.
      • alastairp
        show me where we identify that the token has expired and refresh it?
      • _lucifer
        we try to refresh the token once before handing it to spotify and once after it.
      • this refresh does not propogate to spotipy according to my understanding
      • we also try here https://github.com/metabrainz/listenbrainz-serv... . this one is before we send to spotipy so this one would be visible
      • if the token expired between the first and second check, the failure we see would happen.
      • alastairp
        that pattern of passing in a function from a class instance add some weird complexity
      • _lucifer
        true.
      • i'll simplify this stuff. do you think this hypothesis could be correct ?
      • CatQuest
        prabal: oh cool! thank you!
      • alastairp
        any thoughts on why it wasn't a problem before?
      • _lucifer
        maybe the first check worked earlier and now it doesn;t
      • ruaok
        voodoo!
      • _lucifer
        or sheer coincidence of timing
      • `token_expires < now() as token_expired`
      • 🤦
      • alastairp
        interesting, but that goes all the way back to my original version
      • so we weren't using that value?
      • _lucifer
        oh wait, that's right actually.
      • alastairp
        but domain.Spotify.token_expired() actually recalculates it
      • _lucifer
        that's removed in the new version.
      • alastairp
        OK, I now understand what you're saying _lucifer. if the token has already expired, we refresh it and then go ahead with the update data load. this seems to work fine
      • but if it hasn't expired, but then by the time that we get to it it has, we fail to refresh it properly
      • _lucifer
        yes right
      • alastairp
        `user['token_expired']` is actually a bad way of doing it - because this is true as of the moment that we read the database, not necessarily at the point that we process the user
      • _lucifer
        according to what i see, this process has been broken since ever.
      • right.
      • i think we should not try to refresh it earlier, try to load data and then refresh correctly
      • alastairp
        I think we should do it the other way around. keep this check, but compare against now() and the actual expiry time. add in a 10 minute leeway so that we refresh it before it expires
      • LB-891 - from what I saw here it takes 7 minutes to rotate through all users as well
      • BrainzBot
        LB-891: Check that spotify importer is keeping up with user listens https://tickets.metabrainz.org/browse/LB-891
      • _lucifer
        that might also explain why it worked earlier but not now. earlier the gap was only a few seconds, now the gap could be of almost ~7 min
      • alastairp
        yeah
      • _lucifer
        if we want to pre fetch refresh token, i think we should remove the check to refresh it during data load
      • alastairp
        I think those are 2 clear improvements we can make: be smarter about checking the expiry of a token before we process; be smarter about the refresh when there is an error and how we call this function
      • mmm, interesting proposal