Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add log config option + socialspy for /tpr #4617

Closed
wants to merge 12 commits into from

Conversation

JRoy
Copy link
Member

@JRoy JRoy commented Nov 6, 2021

Adds a config option to tpr.yml to log teleport locations in the console

log-tprs: true

Adds a custom social spy handler for /tpr which gives the destination location.

Att #4613

@JRoy JRoy added type: enhancement Features and feature requests. module: main Issues or PRs for the main Essentials module labels Nov 6, 2021
@JRoy JRoy added this to the 2.19.1 milestone Nov 6, 2021
@pop4959
Copy link
Member

pop4959 commented Nov 6, 2021

These changes feel a bit much to me for how niche of a use case this is. It's already possible to see a player's location in-game using commands such as /whois after the player teleports. For debugging purposes, I think it would be sufficient if the exact location was simply logged to the /ess debug output.

If @Bobcat00 wants to they could also add a listener for UserRandomTeleportEvent to set up custom logging / monitoring.

@JRoy
Copy link
Member Author

JRoy commented Nov 6, 2021

These changes feel a bit much to me for how niche of a use case this is. It's already possible to see a player's location in-game using commands such as /whois after the player teleports. For debugging purposes, I think it would be sufficient if the exact location was simply logged to the /ess debug output.

If @Bobcat00 wants to they could also add a listener for UserRandomTeleportEvent to set up custom logging / monitoring.

I thought about adding this as a debug print but I feel like it's something that should be allowed outside of spamming the console full of the other debug prints.

@pop4959
Copy link
Member

pop4959 commented Nov 6, 2021

I can understand that argument but it just feels a bit odd to me that this would justify adding a special case to social spy, which has no other special cases outside of messaging commands for which it was primarily designed to be used with.

In addition, with the same line of reasoning it would make a whole lot of sense to add logging of locations for all other teleportation commands as well, rather than just this one. I don't see how the use case is much different from wanting to know where a player went when they executed a similar command like /home, /tpaccept, or so on.

@JRoy
Copy link
Member Author

JRoy commented Nov 6, 2021

I can understand that argument but it just feels a bit odd to me that this would justify adding a special case to social spy, which has no other special cases outside of messaging commands for which it was primarily designed to be used with.

In addition, with the same line of reasoning it would make a whole lot of sense to add logging of locations for all other teleportation commands as well, rather than just this one. I don't see how the use case is much different from wanting to know where a player went when they executed a similar command like /home, /tpaccept, or so on.

I'm not opposed to adding those special cases for socialspy inside of #3801

@Bobcat00
Copy link
Contributor

Bobcat00 commented Nov 6, 2021

In addition, with the same line of reasoning it would make a whole lot of sense to add logging of locations for all other teleportation commands as well, rather than just this one. I don't see how the use case is much different from wanting to know where a player went when they executed a similar command like /home, /tpaccept, or so on.

With /tpa, /tpahere, and /tpaccept, you have a player issuing a command at the destination. So that can be logged by a logging plugin like Prism, allowing admins to determine the destination after the fact. With /tpr, there's no player issuing a command at the destination for it to be logged.

Also, the logging could be useful to server owners who want to debug their /tpr setup. Maybe people are ending up in places that he didn't intend. (It's random!) The logging is how he could figure out what's wrong.

But anyway, that's just me. If you guys hate it, that's OK.

I'd just add a one-line output to the log after the teleport, and leave it at that. No SS stuff.

@pop4959
Copy link
Member

pop4959 commented Nov 6, 2021

I'm not opposed to adding those special cases for socialspy inside of #3801

I should have been more specific but I'm not sure if this really belongs in social spy at all, although I guess that might be a bit subjective. I think it'd be worth having another opinion on this, but I think a different solution (such as the notify permissions Bobcat mentioned) would probably make more sense.

With /tpa, /tpahere, and /tpaccept, you have a player issuing a command at the destination. So that can be logged by a logging plugin like Prism, allowing admins to determine the destination after the fact. With /tpr, there's no player issuing a command at the destination for it to be logged.

It doesn't make sense for Essentials to be designed around what features other plugins do or don't have. It sounds like the real issue here is that you want to see the location later, not in real time (e.g. with /whois, social spy, or teleporting to the player to see their location). In that case it probably does make sense to do custom logging to file, or hook into Prism if it supports custom logging, using the UserRandomTeleportEvent.

Also, the logging could be useful to server owners who want to debug their /tpr setup. Maybe people are ending up in places that he didn't intend. (It's random!) The logging is how he could figure out what's wrong.

+1 as to why it might make sense to add to ess debug, but personally I've usually just used the F3 to debug it.

But anyway, that's just me. If you guys hate it, that's OK.

I'd just add a one-line output to the log after the teleport, and leave it at that. No SS stuff.

I don't hate it but I agree that it probably doesn't need to be part of social spy.

@mdcfe
Copy link
Member

mdcfe commented Nov 20, 2021

I have no real objections to adding this, but I'm also not convinced that this has that much purpose. I'm open to other people's opinion on whether this is worth adding or not, but I'm dropping this from the 2.19.1 milestone until we have a consensus.

@mdcfe mdcfe removed this from the 2.19.1 milestone Nov 20, 2021
@@ -39,6 +39,10 @@ public void reloadConfig() {
cachedLocations.clear();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more grammatical sense to have this as “shouldLogTprs”?

@mdcfe mdcfe added this to the 2.20.0 milestone Feb 28, 2022
@mdcfe
Copy link
Member

mdcfe commented Feb 28, 2022

Adding to 2.20 milestone pending a decision on whether this is considered a worthwhile addition.

@Bobcat00
Copy link
Contributor

Bobcat00 commented Mar 1, 2022

Well here's a use case mentioned by @pop4959(!) in #3499:

If players are constantly teleporting to the edge of the world border instead of an actual random location, it's a huge hint to the server owner that they have something set up incorrectly.

Since the TPR destination is not logged, there's no reliable way for the server owners to know that players are constantly teleporting to the edge of the world border instead of an actual random location. Logging each TPR destination would have helped in that case, and it's certainly possible it could help in investigating future problems.

Although as I said above, I'd just do a one-line write to the log file instead of adding it to Social Spy, where it arguably doesn't belong.

@JRoy JRoy modified the milestones: 2.20.0, 2.21.0 Apr 28, 2023
@JRoy JRoy closed this Nov 11, 2023
@JRoy JRoy deleted the feat/log-tpr branch November 11, 2023 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: main Issues or PRs for the main Essentials module type: enhancement Features and feature requests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants