resolves #233 - Disable telescope by default

[zach2825]

I compiled and included telescope assets since they are tiny. Also disabled telescope by default. If you want to use it, you can set TELESCOPE_ENABLED=true in your .env file

[allella]

I'll defer to Bogdan.

Though, since we have new .env options I'm guessing we want to add examples to

.env.example .env.docker .env.testing

Thanks

[zach2825]

I've added some documentation to the README. It's not a good practice to add things to files that should probably be ignored. Also, if someone enables Telescope in their .env.docker file, for example, and accidentally pushes that, it could unintentionally affect others.

Hope that makes sense. .env type files should be ignored for that purpose

[irby]

Whoops, don't have approval to approve. But unit tests are failing, though.

[zach2825]

Whoops, don't have approval to approve. But unit tests are failing, though.

I just needed to merge the latest. I thought I did when I did the work but apparently not lol thanks :+1:

[allella]

@zach2825 the only question I came up with looking at this is if the CSS and JS is being loaded even if the package isn't enabled. Like, is public/vendor/telescope/mix-manifest.json actually going to get added to the CSS / JS loading, or is that only happening on the Telescope URL when it's enabled?

Other than that, I think a :+1: from @bogdankharchenko would be good before we merge this one.

[zach2825]

@zach2825 the only question I came up with looking at this is if the CSS and JS is being loaded even if the package isn't enabled. Like, is public/vendor/telescope/mix-manifest.json actually going to get added to the CSS / JS loading, or is that only happening on the Telescope URL when it's enabled?

Other than that, I think a :+1: from @bogdankharchenko would be good before we merge this one.

No, it should only load the assets if the package is enabled.

[bogdankharchenko]

@zach2825 so I personally feel like we should just close this PR, and set TELESCOPE_ENABLED=false in stage/production.

I do not believe we need to add this service provider and do anything else, since there is already a env which does all of this anyway.

[zach2825]

@bogdankharchenko

Thanks for your input! I understand where you're coming from, but maintaining clarity and justification in our decisions is critical, especially in collaborative environments. Documented reasons for changes help us maintain a consistent approach and make future maintenance and team handovers much smoother.

The proposed changes are backed by specific reasoning, which I'm happy to explain more for greater clarity. It helps ensure that we're all on the same page and that decisions are scalable and understandable by everyone in the team, now and in the future.

If you have concerns or need further explanation of the reasons behind the proposed changes, I'm open to discussing them further. Let's aim for a solution based on well-documented reasoning rather than personal preference. I look forward to your thoughts!

[allella]

Not being well versed in the "Laravel way", I'd be inclined to bring the Telescope docs into the conversation.

It seems to include at least some of the files in this PR, like TelescopeServiceProvider and config/telescope.php are suggested.

@bogdankharchenko are there specific files you'd suggest trimming out as unnecessary?

I'll still defer to you both to find a happy medium.

The main concern was / is to get this disabled on production, since it already consumed 6-7 GBs of logs and started throwing unnecessary cronjob errors.

@zach2825 the Telescope doc's suggest to add the package to the composer.json "dont-discover", which I assume is for performance reasons. Is that something we should add to composer.json?

[allella]

@zach2825 I think we're going to merge this PR given that it follows the docs, that Telescope is supported by Laravel, and nobody is advocating for outright removing the Telescope package.

The main outstanding question is if we need to implement the Telescope doc's suggest to add the package to the composer.json "dont-discover", which I assume is for performance reasons. Is that something we should add to composer.json?

Also, there's a merge conflict, so that would need to be resolved.

Thanks

[zach2825]

I'll resolve the merge conflict shortly, as soon as I have a moment. :)

Just wanted to mention, we've recently encountered an interesting issue with a Livewire component. If Telescope had been enabled in production, it could have been helpful in diagnosing the problem. It provides a consolidated view of logs, requests, and exceptions all in one place.

The original concern about the Telescope database table growing too large has been addressed by implementing a cron job that cleans it out. It's a tool that definitely makes troubleshooting more straightforward.

From what I understand, the original issue was somewhat of an overreaction. We discovered a small problem with a large table due to a misconfiguration, and the immediate response was to remove the entire tool without considering its potential benefits. Telescope allows you to click on a request and view all the resources associated with it, including queries and more. Check out the attached screenshot for more details.

I say this because I don't want to force anything into the application. I added it when I first did the rewrite because I see the value. But, it would be nice for others to see it as well and actually give it a chance.

[allella]

@zach2825

I'm my haste to put up an issue I put "Remove Telescope Package" on the issue and it should have more appropriately been "Remove Telescope Package from Production". I wasn't aware of Telescope at all and the first impression with error logs and database timeouts had me in 'fix the immediate problem'-mode.

The core of the issue was likely the lack of --no-dev in handle-deploy-update.sh, which has been update in https://github.com/hackgvl/hackgreenville-com/pull/237

Stage and production still had Telescope running after running composer install --no-dev manually, which is sort of curious, but adding TELESCOPE_ENABLED=false in the .env had the intended effect.

Since you're using Telescope, and it has obvious virtues for development, we'll continue with it with the knowledge and documentation of it being easily turned on and off.

This PR gives us more control than is needed, but since this is a Laravel app, and since the PR follows the Telescope docs, I think we can agree this is the happy middle ground on leaving Telescope in play, but not using it in production.

Thanks again. I'll take another look at merge if there are no more questions.

[allella]

@zach2825 your input on https://github.com/hackgvl/hackgreenville-com/pull/236#issuecomment-1984901230 is still of interest.

Telescope doc's suggest to add the package to the composer.json "dont-discover" and we're not doing that. If it's not needed, then no problem, but I wanted to confirm a yes or no since their docs don't explain the pros and cons.

Thanks

[zach2825]

I've added the line @allella

But, to be frank, I don't like the idea of removing the telescope from production. I hoped that my last reply would spark some conversation that would change the scope of work done in this PR to actually fix the original problem. But, as of now, the PR should remove it from production. It will not clean up the table; that will have to be done manually.

[allella]

@zach2825

Again, please forgive my ignorance. I don't work with Laravel all day and I didn't read the entire Telescope docs. I'm trying to understand how we intend to use it through the context of this conversation.

My interpretation of an earlier comment was that we were in agreement that Telescope was not intended to be run in production. Or, at least that adding --no-dev was expected, which would effectively mean we'd not be using it in production.

With this PR, it seems we'd still have the option to run Telescope in production with a .env and composer command.

Though, given Telescope's documentation talking about "local development" and putting up big warnings about exposing Telescope in production, I've gathered we were keeping Telescope in for development.

Are you suggesting we should plan to use it in stage or production?

I can certainly manually remove the tables from production and stage if those are not likely to be used. As it is, I was able to purge them, so they aren't really causing any problems. Though, I did disable Telescope, at least for now, since there is no cron job to purge the bloat until this PR gets merged and deployed.

[zach2825]

Personally, I like it in all environments. Any time there is an issue in prod, you can go into the telescope dashboard and see the complete request, all resources used in that request, and any relevant logs in one dashboard. Maybe the work in this PR to remove it would work for now, and then in the next meeting, we can talk about it. I can also include a demo of it.

Aside - We can add other gates/checks to limit what users can see for those requests and resources.

[allella]

Yeah, I think it would be good to use it in dev, get everyone familiar with the capabilities and security aspects, and we can work our way into stage or production if/as needed.

We have an issue started to do better log visibility and alerts as a next step to being aware of issues on production so we even know we need to debug them.

The awareness and alerting of errors is a good first step as it's not uncommon that we deploy and errors emerge in the scheduled jobs and go unnoticed until I manually happen to check the logs days or weeks later.

If we have issue debugging production errors in dev or stage, then we can work to go level 2 with Telescope in production.

[allella]

@zach2825 this PR was merged, but it seems composer isn't happy about something. Like maybe the first error gives a clue related to the dont-discover ?

Script @php artisan package:discover handling the post-autoload-dump event returned with error code 1

[allella]

There are some general suggestions to run composer update or other composer commands, but I'll defer to you before messing around.

[allella]

Also, the .env is set to

APP_ENV=staging

[zach2825]

Lame thanks for merging that. I'm not sure why it's complaining, I'll check it out as soon as I can. I did do a test fresh composer install but with telescope enabled I'm my .env file. It'll disable it and retry.

On Mon, Mar 25, 2024, 11:13 AM Jim Ciallella @.***> wrote:

Also, the .env is set to

APP_ENV=staging

— Reply to this email directly, view it on GitHub https://github.com/hackgvl/hackgreenville-com/pull/236#issuecomment-2018237350, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA64TKO2CP4XNDF45TJD5C3Y2A5IJAVCNFSM6AAAAABDY7HWV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJYGIZTOMZVGA . You are receiving this because you were mentioned.Message ID: @.***>