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

telemetry = false in et.cfg does not disable telemetry #504

Closed
fignew opened this issue Mar 20, 2022 · 3 comments
Closed

telemetry = false in et.cfg does not disable telemetry #504

fignew opened this issue Mar 20, 2022 · 3 comments

Comments

@fignew
Copy link
Contributor

fignew commented Mar 20, 2022

Hello,

When running
$ et -v 7 --logtostdout 127.0.0.1
I get the following output:

[INFO 2022-03-20 15:14:56,847 client-main ParseConfigFile.hpp:1400] unsupported config line:   ForwardX11 yes, ignored
[INFO 2022-03-20 15:14:56,847 client-main ParseConfigFile.hpp:1400] unsupported config line:   VisualHostKey yes, ignored
[INFO 2022-03-20 15:14:56,847 client-main ParseConfigFile.hpp:1181] /etc/ssh/ssh_config.d/*.confnot found
[INFO 2022-03-20 15:14:56,847 client-main TerminalClientMain.cpp:189] Parsed ssh config file, connecting to 127.0.0.1
[V1 2022-03-20 15:14:56,847 client-main TerminalClientMain.cpp:14] Connecting
[V4 2022-03-20 15:14:56,847 client-main TcpSocketHandler.cpp:83] Before selecting sockFd
[V4 2022-03-20 15:14:56,847 client-main TcpSocketHandler.cpp:87] sockFd 6is selected6
[INFO 2022-03-20 15:14:56,847 client-main TcpSocketHandler.cpp:107] Error connecting with 127.0.0.1: 111 Connection refused
[INFO 2022-03-20 15:14:56,848 client-main TcpSocketHandler.cpp:141] No host found
[V1 2022-03-20 15:14:56,848 client-main TerminalClientMain.cpp:17] Could not connect to host
Could not reach the ET server: 127.0.0.1:2022

Shutting down sentry

Seeing "Shutting down sentry" indicates to me that telemetry is still enabled

My et.cfg file is the following:

;######################################################################;
;                                                                      ;
;              THIS FILE IS MANAGED BY SALT - DO NOT EDIT              ;
;                                                                      ;
; The contents of this file are managed by Salt. Any changes to this   ;
; file may be overwritten automatically and without warning.           ;
;######################################################################;

[Networking]
port = 2022
# bind_ip = 0.0.0.0

[Debug]
verbose = 0
silent = 0
logsize = 20971520
telemetry = false

I've also tried telemetry = 0 and placing the option before and in the [Networking] section.

@MisterTea
Copy link
Owner

The problem is that the print you are referring to is outside of the #define that disables telemetry:

cerr << "Shutting down sentry" << endl;

Please submit a PR that moves that print one line down. Even without your PR, the telemetry is disabled.

@fignew
Copy link
Contributor Author

fignew commented Mar 21, 2022

Hi MisterTea,

Thanks for your quick response. Very much enjoying my start of using et.

There are two commits in my pull request

The first one is the one mentioned above
And the second is to document the config option. I was having issues with telemetry = false (coredump) but telemetry = 0 works

Thanks again
<Thomas-

MisterTea pushed a commit that referenced this issue Mar 22, 2022
* Only print sentry shutdown message if TelemetryService exists

Fix for #504

* Config example for telemetry option
@jshort
Copy link
Collaborator

jshort commented Dec 13, 2022

I'm going to close this but I still have a small qualm with the ini file telemetry config:

telemetry = bool(stoi(ini.GetValue("Debug", "Telemetry", "1")));

The cxxopts option is assumed to be a boolean but the ini parser logic uses stoi to convert the parsed char* to an int. We should probably support both literal booleans and integer equivalents (0 - false, non-zero - true).

@jshort jshort closed this as completed Dec 13, 2022
jshort added a commit to jshort/EternalTerminal that referenced this issue Dec 13, 2022
In issue MisterTea#504, it was brought to attention that while the command line
arg for enabling telemetry was boolean, the ini config in et.cfg needed
to be an int to change the value.  SimpleIni has a GetBoolValue function
that treats the strings "0", "off", "no", "false" as boolean false and
"1", "on", "true" as boolean true.

Also, this will make telemetry opt-in as opposed to opt-out as has been
requested by multiple users.  The supplied et.cfg file will still have
telemetry on.
jshort added a commit that referenced this issue Jan 5, 2023
In issue #504, it was brought to attention that while the command line
arg for enabling telemetry was boolean, the ini config in et.cfg needed
to be an int to change the value.  SimpleIni has a GetBoolValue function
that treats the strings "0", "off", "no", "false" as boolean false and
"1", "on", "true" as boolean true.

Also, this will make telemetry opt-in as opposed to opt-out as has been
requested by multiple users.  The supplied et.cfg file will still have
telemetry on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants